1827612 - Investigate not using stable hashing for WeakMap tables

Status: RESOLVED WONTFIX

(Core :: JavaScript: GC, enhancement, P1)

Description

[Jon Coppeard (:jonco)]

Currently we use stable hashing (a.k.a. unique IDs) for weak map hash tables. This is simpler but requires extra work maintaining the association between GC things and their stable IDs.

jandem suggested investigating going back to the original scheme of hashing based on the GC pointer. This is more complicated because we have to update the table when GC things move but could make weak map accesses faster.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #0)

jandem suggested investigating going back to the original scheme of hashing based on the GC pointer. This is more complicated because we have to update the table when GC things move but could make weak map accesses faster.

I was curious about doing this as a quick experiment to see how it affects a micro-benchmark for bug 1821107. I don't know if it's the best way to fix this; another approach would be to attach unique ids for cells to arenas somehow...

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #1)
For me the benchmark result goes from 74ns to 43ns, which is quite a saving. The patch is still WIP because it cases test failures for the debugger and I'm not sure why. It's worth noting that this approach potentially adds overhead to minor GC too.

Comment 3

[Jon Coppeard (:jonco)]

We're not currently pursuing this approach.

Comment 4

[Jon Coppeard (:jonco)]

I resurrected the patches to do while investigating other performance issues, so I'll post them here. There are some test failures in weak marking so these are unfinished.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: WIP: Bug 1827612 - Part 1: Move barrier wrappers into WeakMap class

We always use the same wrappers (currently HeapPtr) so this doesn't have to be
part of the template parameters.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: WIP: Bug 1827612 - Part 1: Don't forward arguments to WeakMap methods

This is useful when a type can be moved, but here the types are always GC thing
pointers or Values so there's no point doing this.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: WIP: Bug 1827612 - Part 2: Remove stable hashing for WeakMap and rekey entries instead

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 9

[Jon Coppeard (:jonco)]

I've got these patches working. However the results are not as good as hoped. The time taken for most operations is improved by 5-10%, except on macOS the get operation on an existing entry is slower by 11%.

Mean times over 30 runs for WeakMap.set (add) and WeakMap.get(hit/miss):

         Pre /ns:  Post /ns:  Difference:
macOS
  Add:   165.2849  155.0918   -6.2%
  Hit:   17.50359  19.55429   11.7% 
  Miss:  9.87418   8.683271   -12.1%
Linux
  Add:   223.0492  199.659    -10.5%
  Hit:   30.93277  28.96686   -6.4%
  Miss:  12.90516  12.5175    -3.0%

Comment 10

[Phabricator Automation]

Comment on attachment 9407229 [details]
WIP: Bug 1827612 - Part 1: Don't forward arguments to WeakMap methods

Revision D213580 was moved to bug 1983749. Setting attachment 9407229 [details] to obsolete.