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.

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

[Refactor] improve performance of rule merging #3281

Merged
merged 1 commit into from May 16, 2022

Conversation

golopot
Copy link
Contributor

@golopot golopot commented May 2, 2022

The performance for merged rule in Component.detect is improved. In an end-to-end linting benchmark this change is 4% faster.

Benchmark (mean of 5 runs):

- 5.164 s
+ 4.966 s

@codecov
Copy link

codecov bot commented May 2, 2022

Codecov Report

Merging #3281 (4bc7669) into master (c8833f3) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Current head 4bc7669 differs from pull request most recent head 438c21a. Consider uploading reports for the commit 438c21a to get more accurate results

@@            Coverage Diff             @@
##           master    #3281      +/-   ##
==========================================
- Coverage   97.72%   97.72%   -0.01%     
==========================================
  Files         123      123              
  Lines        8745     8744       -1     
  Branches     3170     3165       -5     
==========================================
- Hits         8546     8545       -1     
  Misses        199      199              
Impacted Files Coverage Δ
lib/util/Components.js 98.72% <100.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8833f3...438c21a. Read the comment docs.

allKeys.forEach((instruction) => {
updatedRuleInstructions[instruction] = (node) => {
if (instruction in detectionInstructions) {
detectionInstructions[instruction](node);
Copy link
Contributor Author

@golopot golopot May 2, 2022

Choose a reason for hiding this comment

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

This property access performed for each AST node turned out to be performance bottleneck.

for (const key of handlersByKey.keys()) {
const fns = handlersByKey.get(key);
rule[key] = function mergedHandler(node) {
for (const fn of fns) {
Copy link
Member

@ljharb ljharb May 2, 2022

Choose a reason for hiding this comment

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

for-of is slow; fns.forEach((node) => fn(node)) is almost certainly faster (and cleaner; loops are bad)

Copy link
Contributor Author

@golopot golopot May 8, 2022

Choose a reason for hiding this comment

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

for-of is not slow in node, v8 have done optimizations to make it performs comparable to for loop.

const N = 100000;
const length = 100;

const array = new Array(length).fill().map((_, i) => i);

function benchmarkForLoop() {
  let start = performance.now();
  let x = 0;
  for (let i = 0; i < N; i++) {
    for (let j = 0; j < array.length; j++) {
      x = (x + array[j]) % 7;
    }
  }
  let end = performance.now();
  console.log(end - start);
}

function benchmarkForOfLoop() {
  let start = performance.now();
  let x = 0;
  for (let i = 0; i < N; i++) {
    for (const y of array) {
      x = (x + y) % 7;
    }
  }
  let end = performance.now();
  console.log(end - start);
}

function benchmarkForEach() {
  let start = performance.now();
  let x = 0;
  for (let i = 0; i < N; i++) {
    array.forEach((y) => {
      x = (x + y) % 7;
    });
  }
  let end = performance.now();
  console.log(end - start);
}

benchmarkForLoop();
benchmarkForOfLoop();
benchmarkForEach();

// Output:
// 42.265081003308296
// 47.21948900818825
// 57.372923001646996

Copy link
Member

@ljharb ljharb May 8, 2022

Choose a reason for hiding this comment

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

Try those benchmarks without using Array(), which creates a sparse array that .fill may not be able to un-taint.

Copy link
Contributor Author

@golopot golopot May 16, 2022

Choose a reason for hiding this comment

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

Done!


/** @type {{[key: string]: Function}} */
const rule = {};
for (const key of handlersByKey.keys()) {
Copy link
Member

@ljharb ljharb May 2, 2022

Choose a reason for hiding this comment

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

same here; let's use handlersByKey.forEach here

for (const rule of rules) {
for (const key of Object.keys(rule)) {
const fns = handlersByKey.get(key);
if (!fns) {
handlersByKey.set(key, [rule[key]]);
} else {
fns.push(rule[key]);
}
}
}
Copy link
Member

@ljharb ljharb May 2, 2022

Choose a reason for hiding this comment

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

Suggested change
for (const rule of rules) {
for (const key of Object.keys(rule)) {
const fns = handlersByKey.get(key);
if (!fns) {
handlersByKey.set(key, [rule[key]]);
} else {
fns.push(rule[key]);
}
}
}
rules.forEach((rule) => {
Object.keys(rule).forEach((key) => {
const fns = handlersByKey.get(key);
if (!fns) {
handlersByKey.set(key, [rule[key]]);
} else {
fns.push(rule[key]);
}
});
});

ljharb
ljharb approved these changes May 16, 2022
@@ -247,6 +247,39 @@ function getWrapperFunctions(context, pragma) {
]);
}

// eslint-disable-next-line valid-jsdoc
Copy link
Member

@ljharb ljharb May 16, 2022

Choose a reason for hiding this comment

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

is there no way to have this rule pass with this function's signature?

Copy link
Contributor Author

@golopot golopot May 17, 2022

Choose a reason for hiding this comment

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

That rule has been deprecated for a long time and it lacks support for lots of typescript syntaxes.

Copy link
Member

@ljharb ljharb May 17, 2022

Choose a reason for hiding this comment

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

gotcha; so we'd have to use a similar rule from the typescript plugin?

Copy link
Contributor Author

@golopot golopot May 17, 2022

Choose a reason for hiding this comment

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

You are looking for eslint-plugin-jsdoc .

Copy link
Contributor Author

@golopot golopot May 17, 2022

Choose a reason for hiding this comment

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

typescript plugin does not handle jsdoc as far as I know

@ljharb ljharb merged commit 438c21a into jsx-eslint:master May 16, 2022
242 of 243 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants