Inline Map/Set set
Categories
(Core :: JavaScript Engine: JIT, enhancement, P2)
Tracking
()
People
(Reporter: calixte, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
It seems that Map.prototype.set and Set.prototype.add haven't been inlined in bug #1341265 which was about Map/Set operations.
We'd like use more Map objects in pdf.js but because of some bad performances we don't (see https://github.com/mozilla/pdf.js/commit/3ad6303893365b7ad2e2a723d25498f9f1e2792c for context).
:anba, is it an oblivion ? or is there a good reason to not do that ?
|
||
Updated•3 years ago
|
|
||
Comment 1•3 years ago
|
||
Inlining Map.prototype.set and Set.prototype.add is more tricky, because it may involve memory allocations when adding new entries, which need to happen in the VM and can't be inlined. Furthermore adding entries may require performing GC barriers, which also needs to happen in the VM. And we also need to call into the VM when using non-JSAtom strings as the keys, but this is also true for Map.prototype.{get,has} and Set.prototype.has.
So before attempting to somehow inline these operations, I'd first check if the pdf.js use case is actually applicable for a possible inlined implementation. (This will require modifying the MapObject class to perform some extra tracking/logging to check if all inputs allow to omit performing VM calls.) Alternatively OrderedHashTable could be profiled to check if there are any low hanging fruits which could be optimised. For example OrderedHashTable eagerly allocates its underlying memory structures and has a small initial capacity. Every trunc((8 / 3) * (1 << N)) element triggers a resize of the hashtable, starting at N=1, so adding a 5th, 10th, 21st, 42nd, 85th, etc. element triggers a resize. mozilla::HashMap supports lazy allocation and has a larger initial capacity, so there are fewer reallocations for small maps.
|
||
Comment 2•6 months ago
|
||
With the recent work in Bug 1851662 and dependents, is this still slow for you?
|
Reporter | |
Comment 3•6 months ago
|
||
It's better, see https://github.com/mozilla/pdf.js/pull/19051.
|
|
||
Comment 4•6 months ago
|
||
Looks like this is fixed. I will close this, but feel free to reopen if you disagree!
Description
•