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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox15 + fixed

People

(Reporter: till, Assigned: till)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — 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)
CCWs for strings don't keep alive their target?  Weird.
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)
Attached patch patch, v2 (obsolete) — Splinter Review
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)
Attached patch patch, v3 (obsolete) — Splinter Review
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+
Rebased patch with last nit addressed.
Attachment #626930 - Attachment is obsolete: true
Attachment #627582 - Flags: review+
Keywords: checkin-needed
Blocks: 723350
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3f2ddd82e8
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/4c3f2ddd82e8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.