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

feat: improve performance #2626

Closed
wants to merge 6 commits into from
Closed

feat: improve performance #2626

wants to merge 6 commits into from

Conversation

anshumanv
Copy link
Member

@anshumanv anshumanv commented Apr 14, 2021

What kind of change does this PR introduce?

performance

Did you add tests for your changes?
existing

If relevant, did you update the documentation?
NA

Summary

  • Improve performance
  • inbuiltOptions as object
  • unnecessary iterations purged

Does this PR introduce a breaking change?
no

Other information

@anshumanv anshumanv requested a review from a team as a code owner April 14, 2021 15:04
@anshumanv
Copy link
Member Author

Looking at CI

@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #2626 (b24a8a5) into master (281f8ef) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2626      +/-   ##
==========================================
- Coverage   95.23%   95.19%   -0.05%     
==========================================
  Files          31       30       -1     
  Lines        1512     1499      -13     
  Branches      429      428       -1     
==========================================
- Hits         1440     1427      -13     
  Misses         72       72              
Impacted Files Coverage Δ
packages/serve/src/index.ts 91.22% <100.00%> (ø)
packages/webpack-cli/lib/webpack-cli.js 96.15% <100.00%> (+<0.01%) ⬆️
packages/generators/src/addon-generator.ts 93.18% <0.00%> (ø)
packages/generators/src/utils/copy-utils.ts

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 281f8ef...b24a8a5. Read the comment docs.

@anshumanv
Copy link
Member Author

Looks like some spacing diff in help output

@snitin315
Copy link
Member

Try a rebase.

@anshumanv
Copy link
Member Author

hm looks like no help

@@ -4,7 +4,7 @@ const { run, hyphenToUpperCase } = require('../../utils/test-utils');
const CLI = require('../../../packages/webpack-cli/lib/index');

const cli = new CLI();
const optimizationFlags = cli.getBuiltInOptions().filter(({ name }) => name.startsWith('optimization-'));
const optimizationFlags = Object.values(cli.getBuiltInOptions().coreFlags).filter(({ name }) => name.startsWith('optimization-'));
Copy link
Member

Choose a reason for hiding this comment

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

Util func?

@@ -316,7 +316,7 @@ class WebpackCLI {
return this.builtInOptionsCache;
}

const minimumHelpFlags = [
const minimumHelpFlags = new Set([
Copy link
Member

Choose a reason for hiding this comment

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

Perf here would have such little impact that there's no point. If there is 10000 objects I understand, but this is Max 30, so O(30ns) vs O(30^2) wouldnt make much difference

Copy link
Member Author

Choose a reason for hiding this comment

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

we did a lookup for every flag, there could be ~300 flags

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Ideally we should reduce number of Object.entries/Object.values/Object.keys/etc

For example we can use this for help:

Object.entries(minimumHelpFlags).forEach(([name, meta]) => {
  options[name].helpLevel = 'minimum';
});

So we avoid unnecessary looping over 1k options

@alexander-akait
Copy link
Member

Found more places to improve perf, I will finish it

@anshumanv
Copy link
Member Author

Big thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants