1200734 - Use stable hashing for LiveScopesMap

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: stable_LiveScopesMap-v0.diff

With stable hash codes and automatic barriers, we can remove the manual barriers and rekeying.

Comment 1

[Shu-yu Guo [:shu]]

Comment on attachment 8655558 [details] [diff] [review]
stable_LiveScopesMap-v0.diff

Review of attachment 8655558 [details] [diff] [review]:
-----------------------------------------------------------------

Amazing.

From what I read, MovableCellHasher keeps a table under lock and key and minor GC/compaction knows how to forward existing entries in the table?

Comment 2

[Terrence Cole [:terrence]]

(In reply to Shu-yu Guo [:shu] from comment #1)
> Comment on attachment 8655558 [details] [diff] [review]
> stable_LiveScopesMap-v0.diff
> 
> Review of attachment 8655558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Amazing.

Turns out this patch isn't quite sufficient. This table is weak (why do we not need the key to be ReadBarriered?) so changing the key type to RelocatablePtr adds incidental pre-barriers that keeps the thing alive too long, causing us to fail various leak tests. Turns out most of our tables fall into this category, so I want to come up with something more general and even easier to use than this.

> From what I read, MovableCellHasher keeps a table under lock and key and
> minor GC/compaction knows how to forward existing entries in the table?

Lock no longer required, thanks to your own observation that we can sweep it on the foreground now that it's in the zone. But yes, that's basically correct.

Comment 3

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18c307d94d34

Comment 4

[Terrence Cole [:terrence]]

ReadBarriered now has an implicit post barrier, so this patch should work as-is now. Let's send it to try to find out!

Comment 5

[Terrence Cole [:terrence]]

Okay, maybe not! Looks like adding a copy-constructor for read-barriered is quite dangerous, as this causes us to fire read barriers during any table resize. When we do a CCW::putWrapper, the read barrier marks the object, which calls Proxy::trace, which looks up the thing in the table: boom.

Comment 6

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f0c3db2db6e

Comment 7

[Terrence Cole [:terrence]]

Huh, when testing I happened to notice that we don't actually need any new constructor on ReadBarriered and removing it allows us to run tests that previously failed. Not sure when this changed, but I'd guess the refactor of ReadBarriered, maybe? In any case, we have a no-args constructor, so we should not be getting any implicit constructors that would just skip the barriers. Either way, new try run!

Comment 8

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a40e3194de31

Comment 9

[Terrence Cole [:terrence]]

Well, that failed! The problem is that removing the new constructor also removed the post barriers. Whoops.

Comment 10

[Terrence Cole [:terrence]]

Note: the new try run is:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fbe52f4fc0a

Comment 11

[Terrence Cole [:terrence]]

The behavior we actually want is: copy construction fires the read barrier on the source as that is creating a new edge; move construction "steals" the lifetime management as well, not firing the read on the now-defunct edge. Since tables use move construction internally, we don't fire barriers on random elements when the table happens to resize, but we still get the right barriers when creating the temporary for insertion into the table.

Comment 12

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0132945252d2e0c34dee0fca06465df9fc5d152f
Bug 1200734 - Use stable hashing for LiveScopesMap; r=shu

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0132945252d2