1132286 - Simplify new object cache allocation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 3.1_simplify_noc_lookups-v0.diff

This pushes the retry down one level, now that we don't need to do the rooting dance in the callers. I think we can probably push down another level, but it's going to require some care, so one layer at a time.

Comment 1

[Terrence Cole [:terrence]]

Try run with the stack looks green --  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8328b81b7de

This is hot code and even has some some specific tests exercising it, so I'm pretty confident that this patch works as expected.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8563129 [details] [diff] [review]
3.1_simplify_noc_lookups-v0.diff

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

::: js/src/jsobj.cpp
@@ +1541,2 @@
>          cache.fillGroup(entry, group, allocKind, &obj->as<NativeObject>());
> +    }

I don't follow the gcNumber logic. I can't figure out what it's protecting against.

If the cache lookup fails, then entry will be -1, and gcNumber will never matter. So why bother recording it? And in what path would a GC make it invalid to populate the cache anyway? I don't see pointers here that could be moved.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8563129 [details] [diff] [review]
3.1_simplify_noc_lookups-v0.diff

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

::: js/src/jsobj.cpp
@@ +1541,2 @@
>          cache.fillGroup(entry, group, allocKind, &obj->as<NativeObject>());
> +    }

Ok, I get it now. Then this needs comments. It's way too subtle.

Except it sounds like you may be doing something different. If not, then at least comment that EntryIndex is invalidated across a GC. And there's probably something worth saying about how it always populates the cache, but I'm not sure what.

Comment 4

[Terrence Cole [:terrence]]

I just added a comment to my new patch that would be very helpful. I'll backport some form of that to this patch.

Comment 5

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8328b81b7de
https://hg.mozilla.org/integration/mozilla-inbound/rev/4638a1552904

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4638a1552904