Closed Bug 1132286 Opened 6 years ago Closed 6 years ago

Simplify new object cache allocation

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attachment #8563129 - Flags: review?(sphink)
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 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.
Attachment #8563129 - Flags: review?(sphink)
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.
Attachment #8563129 - Flags: review+
I just added a comment to my new patch that would be very helpful. I'll backport some form of that to this patch.
Depends on: 1132706
Blocks: 1132706
No longer depends on: 1132706
https://hg.mozilla.org/mozilla-central/rev/4638a1552904
Assignee: nobody → terrence
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.