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

Parser performance improvements #5322

Merged
merged 5 commits into from Sep 11, 2017

Conversation

Projects
None yet
4 participants
@sokra
Member

sokra commented Jul 19, 2017

What kind of change does this PR introduce?
performance

Did you add tests for your changes?
existing tests

If relevant, link to documentation update:
N/A

Summary
Parser.definitions and Parser.renames are using Maps now

Does this PR introduce a breaking change?
Yes

Other information
My first try was using Set for definitions, but this was slow because it's often copied for a nested scope. Because of this I implemented StackedSetMap which is a stacked versions of Set and Map. It keeps a stack of parent scope sets/maps to avoid copying. Asking the StackedSetMap for a value asks all maps in the stack for the value, starting with the top-most one. A found value is "cached" in the top-most map of the stack. Deleting is implemented as set to the default value in the top-most map. Set is implemented as Map with value is a boolean. This makes all operation very performant:

  • creating child scope: O(1)
  • reading a value the first time: O(n) with n is the number of parent scopes
  • reading a value the second time: O(1)
  • writing a value: O(1)
Show outdated Hide outdated lib/Parser.js Outdated
Show outdated Hide outdated lib/Parser.js Outdated
Show outdated Hide outdated lib/Parser.js Outdated
Show outdated Hide outdated lib/Parser.js Outdated
Show outdated Hide outdated lib/Parser.js Outdated
this.prewalkStatement(statement.declaration);
const newDefs = this.scope.definitions.slice(pos);
const newDefs = Array.from(tracker.getAddedItems());
this.scope.definitions = originalDefinitions;
for(let index = 0, len = newDefs.length; index < len; index++) {
const def = newDefs[index];

This comment has been minimized.

@Kovensky

Kovensky Aug 11, 2017

Collaborator

This can be a for (const def of tracker.getAddedItems()) but I don’t know what performance impact it’d have.

@Kovensky

Kovensky Aug 11, 2017

Collaborator

This can be a for (const def of tracker.getAddedItems()) but I don’t know what performance impact it’d have.

This comment has been minimized.

@sokra

sokra Aug 11, 2017

Member

for(;;) is twice as fast as for(of), even in node 8.3.0.

forEach is 40x slower...

@sokra

sokra Aug 11, 2017

Member

for(;;) is twice as fast as for(of), even in node 8.3.0.

forEach is 40x slower...

@webpack-bot webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Aug 11, 2017

@webpack-bot webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Aug 12, 2017

@webpack-bot

This comment has been minimized.

Show comment
Hide comment
@webpack-bot

webpack-bot Aug 12, 2017

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

webpack-bot commented Aug 12, 2017

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra changed the base branch from master to next Sep 11, 2017

@sokra sokra merged commit f53c0ce into next Sep 11, 2017

6 of 9 checks passed

codecov/patch 87.75% of diff hit (target 94.99%)
Details
codecov/project 94.9% (-0.1%) compared to f7bcba7
Details
coverage/coveralls Coverage decreased (-0.05%) to 94.995%
Details
ci/circleci Your tests passed on CircleCI!
Details
codacy/pr Good work! A positive pull request.
Details
codecov/changes No unexpected coverage changes found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@webpack-bot webpack-bot added PR: next and removed PR: non-master labels Sep 11, 2017

@sokra sokra deleted the performance/parser branch Sep 11, 2017

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