Closed
Bug 1244279
Opened 9 years ago
Closed 9 years ago
Investigate stealing a bit on ObjectElements to signify whether the object is in the whole cell buffer
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: fitzgen, Assigned: fitzgen)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
If the object is already in the whole cell buffer, then adding SlotsEdges for that object is just adding unnecessary work.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
Found some cycles in between other things.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8746180 -
Flags: review?(terrence)
Assignee | ||
Comment 4•9 years ago
|
||
Haven't ran the numbers yet, so I don't know if this is a win or not yet. Stay
tuned for more...
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Looks like a win to me!
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
Many header include order! Wow! Such -inl.h!
Attachment #8746225 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Attachment #8746181 -
Attachment is obsolete: true
Attachment #8746181 -
Flags: review?(terrence)
Assignee | ||
Comment 10•9 years ago
|
||
Updated•9 years ago
|
Attachment #8746180 -
Flags: review?(terrence) → review+
Comment 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
Moved the template out to Linker.cpp and explicitly instantiated it with CanGC
and NoGC.
Attachment #8746619 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8746225 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 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•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb98ac7de73d
https://hg.mozilla.org/mozilla-central/rev/796d263c5c19
Status: ASSIGNED → RESOLVED
Closed: 9 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.
Description
•