Investigate stealing a bit on ObjectElements to signify whether the object is in the whole cell buffer

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

If the object is already in the whole cell buffer, then adding SlotsEdges for that object is just adding unnecessary work.
No time to look into this at the moment, but it is still definitely something to investigate when someone gets some cycles.
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
Found some cycles in between other things.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Created attachment 8746180 [details] [diff] [review]
Part 0: Add a GC ubench for large arrays with both elements and properties
Attachment #8746180 - Flags: review?(terrence)
Created attachment 8746181 [details] [diff] [review]
Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

Haven't ran the numbers yet, so I don't know if this is a win or not yet. Stay
tuned for more...
Created attachment 8746196 [details]
ubench with patch
Created attachment 8746199 [details]
ubench without patch
Looks like a win to me!
Comment on attachment 8746181 [details] [diff] [review]
Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aa95f2d55ca
Attachment #8746181 - Flags: review?(terrence)
Created attachment 8746225 [details] [diff] [review]
Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

Many header include order! Wow! Such -inl.h!
Attachment #8746225 - Flags: review?(terrence)
Attachment #8746181 - Attachment is obsolete: true
Attachment #8746181 - Flags: review?(terrence)
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=852228d11305
Attachment #8746180 - Flags: review?(terrence) → review+
Comment on attachment 8746225 [details] [diff] [review]
Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

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

::: js/src/jit/IonCaches.cpp
@@ +5041,5 @@
>      // Monitor changes to cache entry.
>      TypeScript::Monitor(cx, script, pc, vp);
>  
>      return true;
>  }

Accidental?

::: js/src/jit/Linker-inl.h
@@ +57,5 @@
> +    masm.link(code);
> +    if (masm.embedsNurseryPointers())
> +        cx->runtime()->gc.storeBuffer.putWholeCell(code);
> +    return code;
> +}

I cannot image that this much code actually needs to be inlined for performance. I expect it was only in a header to avoid the need to manually instantiate. Please just use a .cpp for this instead of an inl.h.
Attachment #8746225 - Flags: review?(terrence) → review+
Created attachment 8746619 [details] [diff] [review]
Part 1: Take a bit in `ObjectElements::Flags` to indicate whether the object is in the whole cell store buffer

Moved the template out to Linker.cpp and explicitly instantiated it with CanGC
and NoGC.
Attachment #8746619 - Flags: review+
Attachment #8746225 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb98ac7de73d
https://hg.mozilla.org/integration/mozilla-inbound/rev/796d263c5c19
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cb98ac7de73d
https://hg.mozilla.org/mozilla-central/rev/796d263c5c19
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.