1953446 - Optimize handling of weakmaps in major GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Bug 1952626 contains a (pretty artificial) weakmap benchmark where we do very poorly, with a lot time spent in major GC. This bug is about improving the way weakmaps are treated during major GC. The patches improve performance by about 20% in local testing.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 1: Tidy up WeakMap class a little r?sfink

This renames exposeGCThingToActiveJS to valueReadBarrier, consolidtates
protected method definitions and moves a comment about delegates next to the
code it is referring to.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 2: Remove ObjectWeakMap class r?sfink

Previously ObjectWeakMap was a wrapper around a JSObject to JS::Value weakmap
with a slightly different API. This didn't make sense because we actually
support object to object maps directly, so storing the values as JS::Value is
pointless.

The patch removes this and replaces its uses with the standard weakmap
template. A 'maybeGet' method is added to get the value for a ket or a default
constructed Value if it is missing, since this is useful and frequently used.
Better name suggestions accepted.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 3: Optimize finding sweep group edges for weakmaps r?sfink

This caches information about whether a weakmap is likely to contain keys with
delegates or symbol keys so it can hopefully skip scanning the map during GC.

Note that whether a key has a delegate can change if the object is swapped with
a wrapper, so we have to count keys which are able to be swapped too.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 4: Assert that weakmaps have GC lifetime r?sfink

An exception is made for deleting empty maps to allow OOM handling.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 5: Remove barrier wrappers from WeakMap template parameters r?sfink

All clients use HeapPtr anyway. Adding the barriers internally will allow us to
elide them on destruction.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 6: Remove WeakMap's private inheritance of HashMap r?sfink

I used to thing this was neat and made things simpler, but I've come to realise
it does not. Embedding the map in WeakMap gives much more control at the cost
of writing a few simple forwarding methods.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 7: Remove write barriers on weakmap destruction r?sfink

Currently we trigger write barriers when we destroy a weakmap but this is
ununecessary. This removes them by making the actual storage of the map
unbarriered, and casting it to a barriered map for all mutator accesses.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1953446 - Part 8: Remove duplicate read barrier r?sfink

This is already handled in WeakMap::lookup.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51814e663ad1
Part 1: Tidy up WeakMap class a little r=sfink
https://hg.mozilla.org/integration/autoland/rev/ca3098e7a08c
Part 2: Remove ObjectWeakMap class r=sfink
https://hg.mozilla.org/integration/autoland/rev/fe5c84911e61
Part 3: Optimize finding sweep group edges for weakmaps r=sfink
https://hg.mozilla.org/integration/autoland/rev/370d2fb98bcf
Part 4: Assert that weakmaps have GC lifetime r=sfink
https://hg.mozilla.org/integration/autoland/rev/8933ab73f76c
Part 5: Remove barrier wrappers from WeakMap template parameters r=sfink
https://hg.mozilla.org/integration/autoland/rev/c9edd0c87887
Part 6: Remove WeakMap's private inheritance of HashMap r=sfink
https://hg.mozilla.org/integration/autoland/rev/b4fd520a5e13
Part 7: Remove write barriers on weakmap destruction r=sfink
https://hg.mozilla.org/integration/autoland/rev/573a55983065
Part 8: Remove duplicate read barrier r=sfink

Comment 10

[amarc]

Backed out for causing multiple failures

• Backout link
• Push with failures
• Failure Log
• Failure Log 2
• Failure Log 3

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/268e09591fe6
Part 1: Tidy up WeakMap class a little r=sfink
https://hg.mozilla.org/integration/autoland/rev/f86f9ae1d62c
Part 2: Remove ObjectWeakMap class r=sfink
https://hg.mozilla.org/integration/autoland/rev/32a805058a1e
Part 3: Optimize finding sweep group edges for weakmaps r=sfink
https://hg.mozilla.org/integration/autoland/rev/5074cd5ae805
Part 4: Assert that weakmaps have GC lifetime r=sfink
https://hg.mozilla.org/integration/autoland/rev/6af14659fcba
Part 5: Remove barrier wrappers from WeakMap template parameters r=sfink
https://hg.mozilla.org/integration/autoland/rev/4fdf0f4b9ef6
Part 6: Remove WeakMap's private inheritance of HashMap r=sfink
https://hg.mozilla.org/integration/autoland/rev/82f91932ddfb
Part 7: Remove write barriers on weakmap destruction r=sfink
https://hg.mozilla.org/integration/autoland/rev/3cf06f78a8fd
Part 8: Remove duplicate read barrier r=sfink

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/268e09591fe6
https://hg.mozilla.org/mozilla-central/rev/f86f9ae1d62c
https://hg.mozilla.org/mozilla-central/rev/32a805058a1e
https://hg.mozilla.org/mozilla-central/rev/5074cd5ae805
https://hg.mozilla.org/mozilla-central/rev/6af14659fcba
https://hg.mozilla.org/mozilla-central/rev/4fdf0f4b9ef6
https://hg.mozilla.org/mozilla-central/rev/82f91932ddfb
https://hg.mozilla.org/mozilla-central/rev/3cf06f78a8fd