Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reduce memory allocation in rules and utils #2976

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
+708 −514
Draft

Reduce memory allocation in rules and utils #2976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b4cd869
Curry read only props reporting function to avoid creating closures
willheslam May 4, 2021
152a276
Replace Object.keys with values/entries
willheslam May 4, 2021
35a88cb
Avoid allocating empty objects or arrays when possible
willheslam May 3, 2021
69bcb8d
Speed up string manipulation and testing
willheslam May 3, 2021
da9231a
Speed up Components list by avoiding some array allocations and remov…
willheslam May 4, 2021
8eff94d
Speed up Components utils
willheslam May 3, 2021
671341e
Cache lower cased pragma
willheslam May 4, 2021
0f7183e
Replace isFirstLetterCapitalised with regex check
willheslam May 3, 2021
473d7c1
Replace array traversals with object property lookup
willheslam May 3, 2021
ccdd17c
Speed up no-deprecated
willheslam May 3, 2021
a2b5327
Preallocate arrays used as logical or equality checks
willheslam May 4, 2021
d16fcb3
Avoid recomputing expensive React version check
willheslam May 4, 2021
232d6a1
Various speed ups by avoiding work and memory allocations
willheslam May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 25 additions & 22 deletions lib/rules/boolean-prop-naming.js
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@

'use strict';

const values = require('object.values');
const Components = require('../util/Components');
const propsUtil = require('../util/props');
const docsUrl = require('../util/docsUrl');
Expand Down Expand Up @@ -173,17 +174,17 @@ module.exports = {
* @param {Function} addInvalidProp callback to run for each error
*/
function runCheck(proptypes, addInvalidProp) {
proptypes = proptypes || [];

proptypes.forEach((prop) => {
if (config.validateNested && nestedPropTypes(prop)) {
runCheck(prop.value.arguments[0].properties, addInvalidProp);
return;
}
if (flowCheck(prop) || regularCheck(prop) || tsCheck(prop)) {
addInvalidProp(prop);
}
});
if (proptypes && proptypes.length > 0) {
proptypes.forEach((prop) => {
if (config.validateNested && nestedPropTypes(prop)) {
runCheck(prop.value.arguments[0].properties, addInvalidProp);
return;
}
if (flowCheck(prop) || regularCheck(prop) || tsCheck(prop)) {
addInvalidProp(prop);
}
});
}
}

/**
Expand Down Expand Up @@ -226,7 +227,9 @@ module.exports = {
if (!node || !Array.isArray(args)) {
return;
}
args.filter((arg) => arg.type === 'ObjectExpression').forEach((object) => validatePropNaming(node, object.properties));
args.forEach((object) => object.type === 'ObjectExpression'
&& validatePropNaming(node, object.properties)
);
Comment on lines -229 to +232
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how meaningful is this specific change? the engine should be able to optimize this down to the equivalent for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.
I'm not convinced v8 can optimise a filter and forEach into an equivalent for loop - some (admittedly slapdash and micro) benchmarking with node v14.15.3 iterating many times over an array shows a singular forEach with a built-in conditional as reliably faster than a filter, forEach chain:

$ ITERATIONS=100 SIZE=10000 node arraytest-filter-for-each.js
filter, forEach: time taken 58.374 ms
$ ITERATIONS=100 SIZE=10000 node arraytest-just-for-each.js
just forEach: time taken 43.283 ms
$ ITERATIONS=100 SIZE=10000 node arraytest-for-of-loop.js
for of loop: time taken 41.101 ms

The singular forEach is consistently faster - depending on the arrangement of the code seems to be around 20 -> 40% faster.
Going with more iterations but a smaller array shows similar results too, just in case:

$ ITERATIONS=10000 SIZE=100 node arraytest-filter-for-each.js
filter, forEach: time taken 45.790 ms
$ ITERATIONS=10000 SIZE=100 node arraytest-just-for-each.js
just forEach: time taken 40.437 ms
$ ITERATIONS=10000 SIZE=100 node arraytest-for-of-loop.js
for of loop: time taken 36.737 ms

Here's the gist:
https://gist.github.com/willheslam/d45a1d183d5d778d82c5113ffb8a4ab8
(using a naked for loop is faster than all three, but that's probably going too far)

Whether this specific change is meaningful, I don't honestly have concrete answer - it depends on the frequency the function is called, the average size of args (and how many of them fulfil the condition)
and also how much the surrounding environment is generating memory pressure.

Benchmarking each individual change is difficult, given the noise and iteration time and how some effects of memory allocation are only obvious in concert, so as a blanket rule this PR finds any quick, localised opportunities to reduce memory allocation, where the change didn't massively impact the flow of the code.

}

// --------------------------------------------------------------------------
Expand Down Expand Up @@ -312,18 +315,18 @@ module.exports = {
}

const list = components.list();
Object.keys(list).forEach((component) => {
values(list).forEach((component) => {
// If this is a functional component that uses a global type, check it
if (
(
list[component].node.type === 'FunctionDeclaration'
|| list[component].node.type === 'ArrowFunctionExpression'
component.node.type === 'FunctionDeclaration'
|| component.node.type === 'ArrowFunctionExpression'
)
&& list[component].node.params
&& list[component].node.params.length
&& list[component].node.params[0].typeAnnotation
&& component.node.params
&& component.node.params.length
&& component.node.params[0].typeAnnotation
) {
const typeNode = list[component].node.params[0].typeAnnotation;
const typeNode = component.node.params[0].typeAnnotation;
const annotation = typeNode.typeAnnotation;
let propType;
if (annotation.type === 'GenericTypeAnnotation') {
Expand All @@ -336,14 +339,14 @@ module.exports = {

if (propType) {
validatePropNaming(
list[component].node,
component.node,
propType.properties || propType.members
);
}
}

if (list[component].invalidProps && list[component].invalidProps.length > 0) {
reportInvalidNaming(list[component]);
if (component.invalidProps && component.invalidProps.length > 0) {
reportInvalidNaming(component);
}
});

Expand Down
13 changes: 8 additions & 5 deletions lib/rules/default-props-match-prop-types.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@

'use strict';

const values = require('object.values');
const Components = require('../util/Components');
const docsUrl = require('../util/docsUrl');

Expand Down Expand Up @@ -93,11 +94,13 @@ module.exports = {
const list = components.list();

// If no defaultProps could be found, we don't report anything.
Object.keys(list).filter((component) => list[component].defaultProps).forEach((component) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same with this .filter change to an if check inside forEach?

reportInvalidDefaultProps(
list[component].declaredPropTypes,
list[component].defaultProps || {}
);
values(list).forEach((component) => {
if (component.defaultProps) {
reportInvalidDefaultProps(
component.declaredPropTypes,
component.defaultProps || {}
);
}
});
}
};
Expand Down
139 changes: 71 additions & 68 deletions lib/rules/destructuring-assignment.js
View file
Open in desktop
Expand Up @@ -13,6 +13,16 @@ const DEFAULT_OPTION = 'always';
function createSFCParams() {
const queue = [];

const identifyProps = (params) => {
const props = params[0];
return props && !props.destructuring && props.name;
};

const identifyContext = (params) => {
const context = params[1];
return context && !context.destructuring && context.name;
};

return {
push(params) {
queue.unshift(params);
Expand All @@ -21,17 +31,11 @@ function createSFCParams() {
queue.shift();
},
propsName() {
const found = queue.find((params) => {
const props = params[0];
return props && !props.destructuring && props.name;
});
const found = queue.find(identifyProps);
return found && found[0] && found[0].name;
},
contextName() {
const found = queue.find((params) => {
const context = params[1];
return context && !context.destructuring && context.name;
});
const found = queue.find(identifyContext);
return found && found[1] && found.name;
}
};
Expand Down Expand Up @@ -87,24 +91,26 @@ module.exports = {
* FunctionDeclaration, or FunctionExpression
*/
function handleStatelessComponent(node) {
const params = evalParams(node.params);

const SFCComponent = components.get(context.getScope(node).block);
if (!SFCComponent) {
return;
}
const params = evalParams(node.params);

sfcParams.push(params);

if (params[0] && params[0].destructuring && components.get(node) && configuration === 'never') {
context.report({
node,
messageId: 'noDestructPropsInSFCArg'
});
} else if (params[1] && params[1].destructuring && components.get(node) && configuration === 'never') {
context.report({
node,
messageId: 'noDestructContextInSFCArg'
});
if (configuration === 'never') {
if (params[0] && params[0].destructuring && components.get(node)) {
context.report({
node,
messageId: 'noDestructPropsInSFCArg'
});
} else if (params[1] && params[1].destructuring && components.get(node)) {
context.report({
node,
messageId: 'noDestructContextInSFCArg'
});
}
}
}

Expand All @@ -116,15 +122,11 @@ module.exports = {
}

function handleSFCUsage(node) {
const propsName = sfcParams.propsName();
const contextName = sfcParams.contextName();
// props.aProp || context.aProp
const isPropUsed = (
(propsName && node.object.name === propsName)
|| (contextName && node.object.name === contextName)
)
&& !isAssignmentLHS(node);
if (isPropUsed && configuration === 'always') {
const isPropUsed = node.object.name && (
node.object.name === sfcParams.propsName() || node.object.name === sfcParams.contextName()
) && !isAssignmentLHS(node);
if (isPropUsed) {
context.report({
node,
messageId: 'useDestructAssignment',
Expand Down Expand Up @@ -155,8 +157,7 @@ module.exports = {
);

if (
isPropUsed && configuration === 'always'
&& !(ignoreClassFields && isInClassProperty(node))
isPropUsed && !(ignoreClassFields && isInClassProperty(node))
) {
context.report({
node,
Expand All @@ -183,49 +184,51 @@ module.exports = {
'FunctionExpression:exit': handleStatelessComponentExit,

MemberExpression(node) {
const SFCComponent = components.get(context.getScope(node).block);
const classComponent = utils.getParentComponent(node);
if (SFCComponent) {
handleSFCUsage(node);
}
if (classComponent) {
handleClassUsage(node);
if (configuration === 'always') {
const SFCComponent = components.get(context.getScope(node).block);
const classComponent = utils.getParentComponent(node);
if (SFCComponent) {
handleSFCUsage(node);
}
if (classComponent) {
handleClassUsage(node);
}
}
},

VariableDeclarator(node) {
const classComponent = utils.getParentComponent(node);
const SFCComponent = components.get(context.getScope(node).block);

const destructuring = (node.init && node.id && node.id.type === 'ObjectPattern');
// let {foo} = props;
const destructuringSFC = destructuring && (node.init.name === 'props' || node.init.name === 'context');
// let {foo} = this.props;
const destructuringClass = destructuring && node.init.object && node.init.object.type === 'ThisExpression' && (
node.init.property.name === 'props' || node.init.property.name === 'context' || node.init.property.name === 'state'
);

if (SFCComponent && destructuringSFC && configuration === 'never') {
context.report({
node,
messageId: 'noDestructAssignment',
data: {
type: node.init.name
}
});
}
if (configuration === 'never') {
const destructuring = (node.init && node.id && node.id.type === 'ObjectPattern');
// let {foo} = props;
const destructuringSFC = destructuring && (node.init.name === 'props' || node.init.name === 'context');
// let {foo} = this.props;
const destructuringClass = destructuring && node.init.object && node.init.object.type === 'ThisExpression' && (
node.init.property.name === 'props' || node.init.property.name === 'context' || node.init.property.name === 'state'
);

if (destructuringSFC && components.get(context.getScope(node).block)) {
context.report({
node,
messageId: 'noDestructAssignment',
data: {
type: node.init.name
}
});
}

if (
classComponent && destructuringClass && configuration === 'never'
&& !(ignoreClassFields && node.parent.type === 'ClassProperty')
) {
context.report({
node,
messageId: 'noDestructAssignment',
data: {
type: node.init.property.name
}
});
if (
destructuringClass
&& !(ignoreClassFields && node.parent.type === 'ClassProperty')
&& utils.getParentComponent(node)
) {
context.report({
node,
messageId: 'noDestructAssignment',
data: {
type: node.init.property.name
}
});
}
}
}
};
Expand Down
18 changes: 12 additions & 6 deletions lib/rules/display-name.js
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@

'use strict';

const values = require('object.values');
const Components = require('../util/Components');
const astUtil = require('../util/ast');
const docsUrl = require('../util/docsUrl');
Expand Down Expand Up @@ -42,14 +43,16 @@ module.exports = {
const config = context.options[0] || {};
const ignoreTranspilerName = config.ignoreTranspilerName || false;

const markFlag = {
hasDisplayName: true
};

/**
* Mark a prop type as declared
* @param {ASTNode} node The AST node being checked.
*/
function markDisplayNameAsDeclared(node) {
components.set(node, {
hasDisplayName: true
});
components.set(node, markFlag);
}

/**
Expand Down Expand Up @@ -229,9 +232,12 @@ module.exports = {
'Program:exit'() {
const list = components.list();
// Report missing display name for all components
Object.keys(list).filter((component) => !list[component].hasDisplayName).forEach((component) => {
reportMissingDisplayName(list[component]);
});
values(list)
.forEach((component) => {
if (!component.hasDisplayName) {
reportMissingDisplayName(component);
}
});
}
};
})
Expand Down
5 changes: 3 additions & 2 deletions lib/rules/forbid-component-props.js
View file
Open in desktop
Expand Up @@ -83,8 +83,6 @@ module.exports = {
return {
JSXAttribute(node) {
const parentName = node.parent.name;
// Extract a component name when using a "namespace", e.g. `<AntdLayout.Content />`.
const tag = parentName.name || `${parentName.object.name}.${parentName.property.name}`;
const componentName = parentName.name || parentName.property.name;
if (componentName && typeof componentName[0] === 'string' && componentName[0] !== componentName[0].toUpperCase()) {
// This is a DOM node, not a Component, so exit.
Expand All @@ -93,6 +91,9 @@ module.exports = {

const prop = node.name.name;

// Extract a component name when using a "namespace", e.g. `<AntdLayout.Content />`.
const tag = parentName.name || `${parentName.object.name}.${parentName.property.name}`;

if (!isForbidden(prop, tag)) {
return;
}
Expand Down
5 changes: 3 additions & 2 deletions lib/rules/function-component-definition.js
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@

'use strict';

const entries = require('object.entries');
const Components = require('../util/Components');
const docsUrl = require('../util/docsUrl');

Expand All @@ -13,8 +14,8 @@ const docsUrl = require('../util/docsUrl');
// ------------------------------------------------------------------------------

function buildFunction(template, parts) {
return Object.keys(parts)
.reduce((acc, key) => acc.replace(`{${key}}`, parts[key] || ''), template);
return entries(parts)
.reduce((acc, entry) => acc.replace(`{${entry[0]}}`, entry[1] || ''), template);
}

const NAMED_FUNCTION_TEMPLATES = {
Expand Down