Closed Bug 1263771 Opened 4 years ago Closed 3 years ago

Use WeakCache to sweep the inner views table

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed
firefox50 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

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 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+
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/ab87b53b3140
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Yet another patch landed that does not compile under MSVC 2013.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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: 4 years ago4 years ago
Resolution: --- → FIXED
(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.
(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.
(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.
(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.
(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.
(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.
(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.
Depends on: 1265480
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9908e330d178d06ba9af56dc7c9c963688489b
Backout ab87b53b3140 (Bug 1263771) for breaking the MSVC2013 build 2 days before uplift.
(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.
And they have build logs attached. This was certainly not true a couple of months ago; wonder what the rationale is.
Also reopening, as I need to solve the build bustage and reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/476d94850f6a
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1289270
You need to log in before you can comment on or make changes to this bug.