Closed
Bug 758278
Opened 12 years ago
Closed 12 years ago
Sweep crossCompartmentWrappers of all compartments, not only collected ones
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: till, Assigned: till)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file, 3 obsolete files)
5.25 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
Bug 723350 uncovered a problem with CCWs not being collected if only one of the two affected compartments is GC'd. This in turn caused the explosive memory usage described in bug 756549. The attached patch fixes the problem by sweeping CCWs of all compartments, not just the currently collected ones.
Attachment #626861 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 1•12 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=7e351cd119ba
Comment 2•12 years ago
|
||
CCWs for strings don't keep alive their target? Weird.
Updated•12 years ago
|
tracking-firefox15:
--- → +
Comment on attachment 626861 [details] [diff] [review] patch Review of attachment 626861 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Till. I'm glad this fixed the problem. Could you fix these small issues and post a rebased patch? Terrence changes JSCompartment::sweep yesterday and the cross-compartment wrappers are now swept a little differently. It won't affect what you're doing here, though. ::: js/src/jscompartment.h @@ +190,5 @@ > > void *data; > bool active; // GC flag, whether there are active frames > js::WrapperMap crossCompartmentWrappers; > + void sweepCrossCompartmentWrappers(); Please put this after the declaration of sweep. ::: js/src/jsscope.cpp @@ +1183,5 @@ > return nbase; > } > > +/** > + * Remove dead wrappers from the table. Maybe put a longer comment here. Something like "Remove dead wrappers from the table. We must sweep all compartments, since string entries in the crossCompartmentWrappers table are not marked during markCrossCompartmentWrappers." Also, multi-line comments should have this format (with only one star on the first and last line): /* * Text goes here. * Another line. */ @@ +1186,5 @@ > +/** > + * Remove dead wrappers from the table. > + **/ > +void > +JSCompartment::sweepCrossCompartmentWrappers() This function should go in jscompartment.cpp, right after JSCompartment::sweep. The sweepBaseShapeTable function is here because it's related to shapes, and shape stuff usually goes in jsscope.cpp.
Attachment #626861 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 4•12 years ago
|
||
Thanks for the quick review, Bill. Attached is a rebased patch with your comments addressed.
Assignee: general → tschneidereit+bmo
Attachment #626861 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #626925 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 5•12 years ago
|
||
Whoops, wrong patch.
Attachment #626925 -
Attachment is obsolete: true
Attachment #626925 -
Flags: review?(wmccloskey)
Attachment #626930 -
Flags: review?(wmccloskey)
Comment on attachment 626930 [details] [diff] [review] patch, v3 Review of attachment 626930 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. The try results look good to me. I think this is ready. ::: js/src/jscompartment.cpp @@ +519,5 @@ > } > > active = false; > } > +/* Please put an extra line after the end brace.
Attachment #626930 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 7•12 years ago
|
||
Rebased patch with last nit addressed.
Attachment #626930 -
Attachment is obsolete: true
Attachment #627582 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c3f2ddd82e8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox15:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•