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

Add parentheses to factory function of UMD bundles to avoid lazy parsing #3183

Merged
merged 1 commit into from Oct 23, 2019
Merged

Add parentheses to factory function of UMD bundles to avoid lazy parsing #3183

merged 1 commit into from Oct 23, 2019

Conversation

ajihyf
Copy link
Contributor

@ajihyf ajihyf commented Oct 22, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Most modern javascript engines use lazy parsing technique to boost startup performance, which means function will not be fully parsed until it is executed. For IIFE or soon-be-called callback function, we should use parentheses to wrap it to avoid double parsing(lazy parsing + full parsing).

Tested using the previous benchmark https://bl.ocks.org/nolanlawson/raw/b6d57ef2b2ac8147bc6fd300b8183566, unwrapped bundles are a lot slower to load. You can also use this gist https://gist.github.com/ajihyf/d147454691f6d477ba36dac64c13c531 to test locally using node.

Unwrapped Module

Wrapped Module

The benchmarks above are all wrong. We should use eval. Example gist: https://gist.github.com/ajihyf/d1a38e2791d25f702f7c363a1813d563

There was already #774 to solve this issue. However, the latest Rollup lost this functionality. This PR just adds parentheses back and some comments to prevent regression.

@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #3183 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3183   +/-   ##
=======================================
  Coverage   90.19%   90.19%           
=======================================
  Files         165      165           
  Lines        5905     5905           
  Branches     1797     1797           
=======================================
  Hits         5326     5326           
  Misses        350      350           
  Partials      229      229
Impacted Files Coverage Δ
src/finalisers/umd.ts 94.91% <100%> (ø) ⬆️

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 b0d35e1...d1a28f7. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Thanks a lot! Seeing that I am most probably the responsible person, I am really sorry I did not see the context and just thought this was unneeded noise. To my defence, #774 was more than a year before my time at Rollup. So the comments are really needed, as they would have given me pause when I removed the parentheses. Your perf benchmark is very convincing to underline this is definitely a thing!

@lukastaegert lukastaegert merged commit dae509d into rollup:master Oct 23, 2019
7 checks passed
@ajihyf ajihyf deleted the fix_lazy_parsing_regression branch Oct 23, 2019
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.

None yet

2 participants