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

babelPlugin fix proposal and benchmarking setup #1188

Merged
merged 3 commits into from
May 26, 2022

Conversation

naugtur
Copy link
Member

@naugtur naugtur commented May 20, 2022

Note this PR has been opened against naugtur-import-meta-url branch. I wanted to discuss this commit separately

Working on import.meta I stumbled upon a case where if import.meta is used in an export statement it's not being reached by the visitor.
This is one of the possible implementations.

Other ideas include:

  • calling path.traverse inside rewriteDeclaration, which makes the whole thing take 2x the time
  • replacing import.meta in the first ('Analyze') pass, which is controversial.

I think this fix may also fix other issues we didn't discover yet.

I'm aware this has lint issues and a console.log leftover. I'm setting up the PR to make discussion easier

@naugtur naugtur requested review from michaelfig and kriskowal May 20, 2022 13:06
@@ -0,0 +1,53 @@
import Benchmark from 'benchmark';
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if we want this.

I needed something to check if I'm not introducing an awful performance regression.

I'm not super happy with the benchmark. Differences between my implementation and previous implementation were less significant than differences between the same implementation executed at different times.

I tried running both implementations in one suite and warming them up first. I could commit that too, but I doubt we want to keep multiple implementations laying around.

Example:

names correspond with fixtures, 2 means it's a new implementation (specifically one iteration before what's committed here)

small x 698 ops/sec ±11.71% (75 runs sampled)
large x 68.98 ops/sec ±3.19% (63 runs sampled)
exportheavy x 1,032 ops/sec ±4.57% (82 runs sampled)
small2 x 790 ops/sec ±2.57% (82 runs sampled)
large2 x 69.91 ops/sec ±5.07% (60 runs sampled)
exportheavy2 x 1,060 ops/sec ±2.10% (82 runs sampled)

small2 x 632 ops/sec ±11.21% (75 runs sampled)
large2 x 69.29 ops/sec ±2.80% (66 runs sampled)
exportheavy2 x 993 ops/sec ±3.53% (81 runs sampled)
small x 803 ops/sec ±5.18% (78 runs sampled)
large x 75.10 ops/sec ±2.25% (67 runs sampled)
exportheavy x 1,069 ops/sec ±2.15% (84 runs sampled)

With warmup:

warmup x 2,371 ops/sec ±9.89% (78 runs sampled)
small2 x 745 ops/sec ±4.39% (79 runs sampled)
large2 x 70.02 ops/sec ±3.31% (63 runs sampled)
exportheavy2 x 1,038 ops/sec ±2.53% (81 runs sampled)
small x 848 ops/sec ±3.36% (82 runs sampled)
large x 64.16 ops/sec ±8.13% (56 runs sampled)
exportheavy x 939 ops/sec ±6.42% (78 runs sampled)

warmup x 27.49 ops/sec ±7.32% (51 runs sampled)
small2 x 766 ops/sec ±2.59% (79 runs sampled)
large2 x 74.26 ops/sec ±2.04% (64 runs sampled)
exportheavy2 x 984 ops/sec ±4.15% (78 runs sampled)
small x 849 ops/sec ±3.78% (82 runs sampled)
large x 70.81 ops/sec ±5.13% (61 runs sampled)
exportheavy x 896 ops/sec ±9.47% (71 runs sampled)

@@ -0,0 +1,52 @@
export { mapIterable, filterIterable } from './src/helpers/iter-helpers.js';
Copy link
Member Author

Choose a reason for hiding this comment

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

just some code to parse

@@ -0,0 +1,42 @@
import * as babelParser from '@babel/parser';
Copy link
Member Author

Choose a reason for hiding this comment

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

just some code to parse

@naugtur naugtur marked this pull request as draft May 20, 2022 13:24
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

I won't be able to review this for a little while, as it will need some close attention.

Overall, I think doing some of the transformations during the "analyze" phase is not so bad. They are really just "phase1" and "phase2" because I couldn't figure out how to do things in one pass.

@naugtur naugtur requested a review from michaelfig May 25, 2022 18:05
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Thanks for the primer on what you did here. Looks great!

@naugtur naugtur marked this pull request as ready for review May 25, 2022 19:23
naugtur and others added 2 commits May 26, 2022 10:04
Co-authored-by: Michael FIG <mfig@agoric.com>
@naugtur naugtur merged commit bb193c6 into naugtur-import-meta-url May 26, 2022
@naugtur naugtur deleted the naugtur-babel-plugin-visitors branch May 26, 2022 08:47
naugtur added a commit that referenced this pull request May 26, 2022
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

* more cleanup
naugtur added a commit that referenced this pull request May 26, 2022
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

* more cleanup
naugtur added a commit that referenced this pull request Jun 1, 2022
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

* more cleanup
naugtur added a commit that referenced this pull request Jun 2, 2022
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

* more cleanup
naugtur added a commit that referenced this pull request Jun 2, 2022
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

* more cleanup
naugtur added a commit that referenced this pull request Jun 7, 2022
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

* more cleanup
naugtur added a commit that referenced this pull request Jun 13, 2022
…ns in exports + benchmark setup (#1188)

* babelPlugin fix proposal and benchmarking setup

* remove debris

Co-authored-by: Michael FIG <mfig@agoric.com>

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

Successfully merging this pull request may close these issues.

2 participants