Closed
Bug 1263771
Opened 8 years ago
Closed 8 years ago
Use WeakCache to sweep the inner views table
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file)
12.36 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
Cracking open the floodgates. These should be much easier to convert over since everything is already in GC hashtables.
Attachment #8740188 -
Flags: review?(sphink)
Comment 1•8 years ago
|
||
Comment on attachment 8740188 [details] [diff] [review] weakcache_InnerViewTable-v0.diff Review of attachment 8740188 [details] [diff] [review]: ----------------------------------------------------------------- Well, that was pretty painless. An annoying .get(), but that's hardly the end of the world.
Attachment #8740188 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Steve Fink [:sfink:, :s:] from comment #1) > Comment on attachment 8740188 [details] [diff] [review] > weakcache_InnerViewTable-v0.diff > > Review of attachment 8740188 [details] [diff] [review]: > ----------------------------------------------------------------- > > Well, that was pretty painless. An annoying .get(), but that's hardly the > end of the world. Yeah, it's only required in a handful of places: for the about:memory size reporting stuff and Enum. We could pretty reasonably punch through the first via ops and add a wrapper class for the second. Should be easy enough; I'll do it tomorrow.
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8740188 [details] [diff] [review] weakcache_InnerViewTable-v0.diff Review of attachment 8740188 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscompartment.cpp @@ +662,5 @@ > { > globalWriteBarriered = false; > > + if (innerViews.get().needsSweepAfterMinorGC()) > + innerViews.get().sweepAfterMinorGC(); The table we're wrapping here is not a normal GCHashMap, it's a custom class wrapping a GCHashMap. I've added a WeakCacheBase<InnerViewTable> with appropriate passthroughs to avoid the various gets. @@ +1121,5 @@ > tiArrayTypeTables, tiObjectTypeTables, > compartmentTables); > *compartmentTables += baseShapes.sizeOfExcludingThis(mallocSizeOf) > + initialShapes.sizeOfExcludingThis(mallocSizeOf); > + *innerViewsArg += innerViews.get().sizeOfExcludingThis(mallocSizeOf); And here. ::: js/src/vm/ArrayBufferObject.cpp @@ +281,5 @@ > > // Update all views of the buffer to account for the buffer having been > // detached, and clear the buffer's data and list of views. > > + InnerViewTable& innerViews = cx->compartment()->innerViews.get(); Fixed by using auto type for innerViews here and passing through maybeViewsUnbarriered and removeViews. @@ +846,5 @@ > if (!firstView()) { > setFirstView(view); > return true; > } > + return cx->compartment()->innerViews.get().addView(cx, this, view); And we can remove the get() here too by punching through addView. ::: js/src/vm/ArrayBufferObject.h @@ +542,5 @@ > {} > > // Remove references to dead objects in the table and update table entries > // to reflect moved objects. > + static void sweep(InnerViewTable& table) { table.sweep(); } Hey, this is *totally* unnecessary now. Glad I took the time to go over this a second time. @@ +551,5 @@ > return !nurseryKeys.empty() || !nurseryKeysValid; > } > > size_t sizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf); > }; This table is actually only ever contained in a WeakCache now, so no need to use an Ops/MutableOps indirection even.
Assignee | ||
Comment 4•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab87b53b3140fbb5e9e2caa5325ae0d791ee43d4 Bug 1263771 - Sweep the InnerViewTable with WeakCache; r=sfink
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ab87b53b3140
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 6•8 years ago
|
||
Yet another patch landed that does not compile under MSVC 2013.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 7•8 years ago
|
||
I've filed bug 1265158 to track the compilation error. Please don't reopen bugs unless they have been backed out, or don't solve the problem they were filed for!
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 8•8 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #7) > I've filed bug 1265158 to track the compilation error. Please don't reopen > bugs unless they have been backed out, or don't solve the problem they were > filed for! Sorry I let my frustration about the number of things landed in the last 2 days that break MSVC 2013 get the better of me. But, I fully expect this to be backed out on the same basis that the checkin for bug 1232686 were backed out. Also, according to the MDN page on Window builds. 2013 is the compiler to use and 2015 is listed as experimental.
Comment 9•8 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #7) > I've filed bug 1265158 to track the compilation error. Please don't reopen > bugs unless they have been backed out, or don't solve the problem they were > filed for! OH btw thanks for not bothering to CC me on the new bug.
Comment 10•8 years ago
|
||
(In reply to Bill Gianopoulos [:WG9s] from comment #8) > But, I fully expect this to > be backed out on the same basis that the checkin for bug 1232686 were backed > out. That's fine, the sheriff will reopen this bug when it does get backed out :) It just makes bookkeeping harder if we jump the gun, especially with the switch to FF49 right around the corner. > Also, according to the MDN page on Window builds. 2013 is the compiler to > use and 2015 is listed as experimental. Hmm, I think that's just out of date. We're definitely using 2015 in automation now, or this never could have landed in the first place. Support for 2013 won't be dropped until the next cycle at the earliest though, so this does need to be fixed. > OH btw thanks for not bothering to CC me on the new bug. I filed the bug in response to your report; just forgot to CC you.
Comment 11•8 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #10) > (In reply to Bill Gianopoulos [:WG9s] from comment #8) > Hmm, I think that's just out of date. We're definitely using 2015 in > automation now, or this never could have landed in the first place. Support > for 2013 won't be dropped until the next cycle at the earliest though, so > this does need to be fixed. perhaps we should switch the automation windows XP builds to using the oldest supported version on msvc so that things like this get caught earlier.
Comment 12•8 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #10) > > Also, according to the MDN page on Window builds. 2013 is the compiler to > > use and 2015 is listed as experimental. > > Hmm, I think that's just out of date. We're definitely using 2015 in > automation now, or this never could have landed in the first place. Support > for 2013 won't be dropped until the next cycle at the earliest though, so > this does need to be fixed. I updated that page to remove the "Experimental" from 2015 and add "Legacy" to 2013.
Comment 13•8 years ago
|
||
(In reply to Bill Gianopoulos [:WG9s] from comment #11) > (In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #10) > > (In reply to Bill Gianopoulos [:WG9s] from comment #8) > > > Hmm, I think that's just out of date. We're definitely using 2015 in > > automation now, or this never could have landed in the first place. Support > > for 2013 won't be dropped until the next cycle at the earliest though, so > > this does need to be fixed. > perhaps we should switch the automation windows XP builds to using the > oldest supported version on msvc so that things like this get caught earlier. I Might file anew bug on that.
Comment 14•8 years ago
|
||
(In reply to Bill Gianopoulos [:WG9s] from comment #11) > perhaps we should switch the automation windows XP builds to using the > oldest supported version on msvc so that things like this get caught earlier. AFAIK we don't really have "Windows XP" builds - just Win32 and Win64 builds. We run tests on multiple Windows versions though. It's probably best to ask a build peer like gps when we will drop support for VS 2013. Setting up new builds in the meantime may not be worth it.
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9908e330d178d06ba9af56dc7c9c963688489b Backout ab87b53b3140 (Bug 1263771) for breaking the MSVC2013 build 2 days before uplift.
Comment 16•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #14) > (In reply to Bill Gianopoulos [:WG9s] from comment #11) > > perhaps we should switch the automation windows XP builds to using the > > oldest supported version on msvc so that things like this get caught earlier. > > AFAIK we don't really have "Windows XP" builds - just Win32 and Win64 > builds. We run tests on multiple Windows versions though. > > It's probably best to ask a build peer like gps when we will drop support > for VS 2013. Setting up new builds in the meantime may not be worth it. Well we have builds identified on treeherder as Windows XP builds.
Assignee | ||
Comment 17•8 years ago
|
||
And they have build logs attached. This was certainly not true a couple of months ago; wonder what the rationale is.
Assignee | ||
Comment 18•8 years ago
|
||
Also reopening, as I need to solve the build bustage and reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79942c617bac
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/476d94850f6aaea6912cf75afff06e046013797d Bug 1263771 - Sweep the InnerViewTable with WeakCache; r=sfink
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/476d94850f6a
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•