Closed Bug 1224050 Opened 4 years ago Closed 4 years ago

Use stable hashing for InitialShapeTable

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
firefox48 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

This is one of the ugly ones that doesn't show up with a compilation failure because it open codes the hashing of the TaggedProto. I have a patch lying about that didn't work before that I need to rebase. It may "just work" now.
Yup, appears to work now.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8686845 - Flags: review?(jcoppeard)
Comment on attachment 8686845 [details] [diff] [review]
stable_InitialShapeTable-v1.diff

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

::: js/src/vm/Shape.cpp
@@ -1388,5 @@
> -/*
> - * This class is used to add a post barrier on the initialShapes set, as the key
> - * is calculated based on objects which may be moved by generational GC.
> - */
> -class InitialShapeSetRef : public BufferableRef

It's so good to get rid of all of this!

@@ +1459,5 @@
>          return nullptr;
>  
>      Lookup lookup(clasp, protoRoot, nfixed, objectFlags);
> +    if (!p.add(cx, table, lookup, InitialShapeEntry(ReadBarriered<Shape*>(shape),
> +                                                    ReadBarriered<TaggedProto>(protoRoot))))

I think we make the ReadBarriered constructor implicit, so can we just pass shape and protoRoot here?

::: js/src/vm/TaggedProto.cpp
@@ +48,5 @@
> +    HashNumber hn;
> +    if (!obj->zone()->getHashCode(obj, &hn))
> +        oomUnsafe.crash("failed to get hash code for TaggedProto");
> +    return hn;
> +}

It seems like hashCode() duplicates much of the logic in uniqueId().  Can we just call the latter and munge the result?
Attachment #8686845 - Flags: review?(jcoppeard) → review+
A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- octane: EarleyBoyer: -4.8% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=43ca206bb812&tochange=40a37cb11607

More details: http://arewefastyet.com/regressions/#/regression/1799694
https://hg.mozilla.org/mozilla-central/rev/40a37cb11607
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Confirmed fixed with backout
I was not able to reproduce any of the regressions locally, but I applied the same optimizations that worked for the ObjectGroupCompartment::NewTable, so I am hopeful that this will take care of the problem here too.
https://hg.mozilla.org/mozilla-central/rev/676b6db552c8
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1257584
You need to log in before you can comment on or make changes to this bug.