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: convert exportsByName object to map #4362
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4362 +/- ##
==========================================
- Coverage 98.68% 98.68% -0.01%
==========================================
Files 205 205
Lines 7326 7323 -3
Branches 2083 2083
==========================================
- Hits 7230 7227 -3
Misses 36 36
Partials 60 60
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to not: The use of
const foo: Record<string, Something> = Object.create(null);
over
const foo = new Map<string, Something>();
was often done in the past deliberately as objects are noticeably faster with regard to property access and property creation (if it was initialized with Object.create(null)
to trigger V8's "object dictionary mode").
This may not matter much here but it can make a measurable difference in hot code paths with access times being twice as fast for an object (there is just much less stuff happening for objects).
The only advantage I see of this change is that TypeScript is not very good at typing prototype-less objects, so one hast to use "prop" in foo
over foo.hasOwnProperty("prop")
.
But this is something I want you consider as you are touching more and more parts of the code base. Other places where one might consider performance is replacing static object literals such as in knownGlobals.ts
. For a literal that never changes, V8 can create very optimized machine code for property access. The same is just not possible for Sets and Maps. Performance was indeed a consideration for many of the "hot" parts of Rollup.
I will still approve and merge this, but I wanted to draw your attention to this.
src/Chunk.ts
Outdated
@@ -869,13 +865,13 @@ export default class Chunk { | |||
): string { | |||
const hash = createHash(); | |||
hash.update( | |||
[addons.intro, addons.outro, addons.banner, addons.footer].map(addon => addon || '').join(':') | |||
[addons.intro, addons.outro, addons.banner, addons.footer].map(addon => addon ?? '').join(':') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out the fallback is actually not needed (and the Addons
type can be adapted accordingly). I will push a small fix to your branch so that we can merge this.
I thought I think the notion of being a performance bottleneck is outdated: https://v8.dev/blog/hash-code
https://twitter.com/v8js/status/1040219491358179328
I agree with that statement. for a long time I did some initial tests with Objects also don't free memory when properties are being deleted (at least in V8). deleting a property was also historically slower (not sure if that has changed). just a general additional argument, not saying with Maps you also have iterators readily available without needing any additional wrapper functions. granted, one could use the same argument for higher order functions not being available on Maps. I think generally speaking it's fair to say it is recommended to use ... and last but not least, I think the code base would also be consistent across the board. I don't want to convince you, nor change your opinion on that matter. just wanted to give my 2 cents. I have more similar stashes waiting to go into a PR ... let me know if you'd rather prefer me to abstain. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description