Closed
Bug 956081
Opened 11 years ago
Closed 11 years ago
GGC breaks the CC's GC-has-been-run detection.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
The CC uses gcNumber to check if the GC has run[1]. With GGC, we bump the gcNumber outside of "full" GC's, breaking this heuristic. Fortunately, this is only a backup mechanism to prevent manually triggered CC's from running before GC, since normally CC is tied to GC timing.
Since it was added in 2011, the CC has grown js::AreGCGrayBitsValid, which should catch the same case. The flag is initialized false and only set true after a GC. We should double-check the above and then remove the gcNumber check as it is now duplicated.
Also, there is nothing preventing GC number from overflowing. Currently, we GC within 10 seconds of startup anyway, so we're not likely to hit this. However, given how we're changing the GC, this is a loaded and cocked footgun: we should engage the safety.
1- http://dxr.mozilla.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#876
Assignee | ||
Comment 1•11 years ago
|
||
Is this what you had in mind?
https://tbpl.mozilla.org/?tree=Try&rev=01416539d0b4
Attachment #8379240 -
Flags: review?(continuation)
Comment 2•11 years ago
|
||
Comment on attachment 8379240 [details] [diff] [review]
fix_gc_has_been_run_detection-v0.diff
Review of attachment 8379240 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with these changes.
::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +875,5 @@
>
> nsresult
> CycleCollectedJSRuntime::TraverseRoots(nsCycleCollectionNoteRootCallback &aCb)
> {
> + if (!js::AreGCGrayBitsValid()) {
Just change this to something like MOZ_ASSERT(!NeedCollect(), "Cannot cycle collect if GC has not run first!"). The CC is set up in such a way that we should always be okay here. (NeedCollect is just a wrapper around AreGCGrayBitsValid, but it seems a little higher level.) Does that sound okay to you?
If you want to test this, start the browser and then immediately go to about:memory and click on the CC button. (Before the first GC is triggered automatically.)
Attachment #8379240 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•11 years ago
|
Depends on: needcollect
You need to log in
before you can comment on or make changes to this bug.
Description
•