refactor: convert exportsByName object to map

[dnalborczyk]

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

[codecov]

Codecov Report

Merging #4362 (b9a9d7f) into master (9eeb6a0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            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              

Impacted Files	Coverage Δ
src/utils/addons.ts	100.00% <ø> (ø)
src/Chunk.ts	100.00% <100.00%> (ø)
src/utils/exportNames.ts	100.00% <100.00%> (ø)

---

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 9eeb6a0...b9a9d7f. Read the comment docs.

[lukastaegert]

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.

[lukastaegert]

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.

[dnalborczyk]

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).

I thought Object.create() was just being used for historical reasons when Map wasn't invented [yet], not widely available [yet], or just not quite performant [yet].

I think the notion of being a performance bottleneck is outdated: https://v8.dev/blog/hash-code

The SixSpeed benchmark tracks the performance of Map and Set, and these changes resulted in a ~500% improvement.

https://twitter.com/v8js/status/1040219491358179328

In general, don’t be afraid to use modern features! Focus on writing idiomatic code, and let JS engines worry about making it fast.

I agree with that statement. for a long time node.js internal code was also using certain language features to make it fast(er). today that notion has shifted to an idiomatic way. can't find the link to that conversation right now, I think it's buried somewhere in the node.js issues. internals are also using Map wherever possible as far as I can tell (or is being converted if it hasn't already).

I did some initial tests with Object.create and Map. both are more or less on par, with Object.create lookups marginally (microscopically) faster with a very small set, and Map being faster with larger sets. (marginally faster is also an oversimplification. I'm talking 1 million instances being created, 10 items in each, with each 10 items/property lookups, the difference on a 7 year old mac is 300 ms). I can provide some ad-hoc-perf tests if you want.

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 rollup is widely using that feature.

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 Maps for collections, and objects for option bags.

... 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.