Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Sweep crossCompartmentWrappers of all compartments, not only collected ones

RESOLVED FIXED in Firefox 15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: till, Assigned: till)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox15+ fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 626861 [details] [diff] [review]
patch

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

5 years ago
try run: https://tbpl.mozilla.org/?tree=Try&rev=7e351cd119ba
CCWs for strings don't keep alive their target?  Weird.

Updated

5 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

5 years ago
Created attachment 626925 [details] [diff] [review]
patch, v2

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

5 years ago
Created attachment 626930 [details] [diff] [review]
patch, v3

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

5 years ago
Created attachment 627582 [details] [diff] [review]
patch, v4, carrying billm's r+

Rebased patch with last nit addressed.
Attachment #626930 - Attachment is obsolete: true
Attachment #627582 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox15: --- → fixed
You need to log in before you can comment on or make changes to this bug.