1120016 - Allocate short lived JS wrappers in the Nursery

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Terrence Cole [:terrence]]

Attachment: allocate_events_in_the_nursery-v0.diff

The attached patch -- adding the class flag for "Event" objects -- was good enough for testing, but is not really suitable for checking in.

Olli provided some feedback in https://bugzilla.mozilla.org/show_bug.cgi?id=1085597#c11 that I need to grok still.

Comment 1

[Steve Fink [:sfink] [:s:]]

Is there any chance of autodetecting things that could be nursery allocated instead of relying on an explicit [ProbablyShortLivingObject] flag? It'd be nice to keep "can" and "should" separate for understandability. (As in, there are properties of finalizers and whatever that would make it incorrect to nursery allocate, and then there are expected usage patterns that determine whether it's a good idea to nursery allocate.)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Wouldn't almost all the wrappers for the webidl stuff be in "could be nursery allocated" 
category. We just happen to know that Event and Touch objects tend to be short living, 
and Elements and such usually aren't.
Making the wrappers for Element to be allocated from nursery might lead recreating the wrappers
too often (in case Element isn't preserving the wrapper).
So I think we should take advantage of the behavior we know about the platform and which
engine can't easily figure out.

Though, I guess in theory we could have a service which tries to learn about the average
life time of a type of a C++ object and its wrappers and then use that information when
allocating wrappers.

Comment 3

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #1)
> Is there any chance of autodetecting things that could be nursery allocated
> instead of relying on an explicit [ProbablyShortLivingObject] flag? It'd be
> nice to keep "can" and "should" separate for understandability. (As in,
> there are properties of finalizers and whatever that would make it incorrect
> to nursery allocate, and then there are expected usage patterns that
> determine whether it's a good idea to nursery allocate.)

I see this, at least initially, as yet another fairly normal SM heuristic that we're exposing to the browser so that gecko can use its insider knowledge to help us tune. I mean, this is a problem elsewhere too, but the "high infant mortality" heuristic has worked so well so far that we haven't yet had an excuse to worry about it. I expect the same thing will be true here for the foreseeable future.

Comment 4

[Terrence Cole [:terrence]]

Attachment: allocate_events_in_the_nursery-v1.diff

It turns out our tools around webidl are absolutely excellent: this was much easier than I expected.

I verified that the flag was getting set properly on events and that several event related mochitests pass locally in a debug build. Handing to try for a full vetting:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3868ff940c7

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

do we end up inheriting JSCLASS_FINALIZE_FROM_NURSERY to other Event interfaces, like
MouseEvent?

Comment 6

[Terrence Cole [:terrence]]

(In reply to Olli Pettay [:smaug] from comment #5)
> do we end up inheriting JSCLASS_FINALIZE_FROM_NURSERY to other Event
> interfaces, like
> MouseEvent?

Oh, great question! Checking that directly, it looks like no, it did not get inherited. :-(

Comment 7

[Boris Zbarsky [:bzbarsky]]

Could just store the boolean on the IDLInterface and copy from the parent in finish().

Comment 8

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Could just store the boolean on the IDLInterface and copy from the parent in
> finish().

It looks like all the booleans we're currently storing in IDLInterface are relatively core to the idea of an interface: e.g. we've maintained excellent code disciple so far. I'd like to avoid cluttering that up with random extension bools not being in the extensions dict. I think the right way to do this is to do the same thing as {is|get}JSImplementation: add a simple wrapper that does lookup for us. I've got a patch with that appears that seems to work, so I'll clean it up and see if we can get consensus on it.

Comment 9

[Terrence Cole [:terrence]]

Attachment: allocate_events_in_the_nursery-v2.diff

Try run looks reasonable and nursery finalize times are non-zero now: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36fc51964f3f

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8554780 [details] [diff] [review]
allocate_events_in_the_nursery-v2.diff

This is still going off the rails somehow as it seems we're allocating things in the nursery now, but somehow they always end up still alive. A moment, please while I investigate.

Comment 11

[Terrence Cole [:terrence]]

So the issue here is that for some reason the previous patch was missing the hunk in SetObjectWrapper that skips the store buffer update if IsInsideNursery. Moreover, because this hunk was missing, I totally missed the fact that we do actually need to call the op_moved when not finalizing.

After discussion at today's GC meeting, we've decided to add a new WeakHeap primitive that handles these details automatically.

Comment 12

[Terrence Cole [:terrence]]

(In reply to Terrence Cole [:terrence] from comment #11)
> After discussion at today's GC meeting, we've decided to add a new WeakHeap
> primitive that handles these details automatically.

Specifically, as opposed to adding a long and highly confusing comment to SetWrapperObject where we cannot simultaneously assert that the finalizer will always do something reasonable. Even if we only ever use this primitive for WrapperCache, I think the clarity will still be worthwhile.

Comment 13

[Terrence Cole [:terrence]]

Attachment: 1_impl_weakheap_pointer-v0.diff

Comment 14

[Terrence Cole [:terrence]]

Attachment: 2_allocate_events_in_nursery-v3.diff

Comment 15

[Terrence Cole [:terrence]]

Attachment: hax.patch

These are some hacks for performance testing -- getting rid of the duplicate finalization path and raising the store buffer limits so we actually end up doing collections correctly.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

MutationRecords should be also short living objects, and there can be plenty of them.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Will fixing this bug allow Promise js wrappers to be allocated in the nursery?

I am trying to evaluate a possible spec API that heavily uses promises and current benchmarks show a lot of GC/CC activity.  I'm curious if I can reasonably expect this to significantly improve in the future.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Promises always preserve their wrapper, because various code depends on it always existing (e.g. resolving a promise depends on the wrapper existing, and can't tolerate failures to allocate it lazily).  So it doesn't make sense to nursery-allocate them as things stand, because they can't be collected until CC happens anyway.

Comment 19

[Andrew McCreight [:mccr8]]

Olli pointed out that v8 seems to have implemented something like this: "The new minor GC doesn't look up reachability from Blink at all (which means that the minor GC doesn't need to run object grouping!). Alternately, the minor GC collects V8 wrappers that are "unmodified" (i.e., doesn't have any expando, isn't registered to weak maps etc). This is going to make a significant performance improvement to blink_perf benchmarks!"
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Gomn7ANVV0I

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v7 (finalization in Gecko approach)

This is the one which should fail linking, but can't reproduce locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf0dd68b55b

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 8697075 [details] [diff] [review]
v7 (finalization in Gecko approach)

I landed a patch for the link errors in bug 1231964. Did you want feedback for this otherwise? I looked over the patch a bit but not in great depth.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Well, I was thinking just feedback whether you think the approach is totally insane :)
I'm about to update the patch and ask then review.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: events_in_nursery_merge_8.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e3434bb064d
Hopefully this links now.

So this approach adds just a callback to js engine to tell when objects are tenured and CycleCollectedJSRuntime uses that information to either
call finalizer or do nothing.
Note, DOM objects finalization is deferred, so it doesn't happen during minorGC but soon after that.


One thing we could perhaps optimize if we take this is to treat 'refcnt == 1 and object waiting to be finalized' effectively as snowwhite object so that CC doesn't put those objects to CC graph if CC runs before deferred finalization runnable.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 8700424 [details] [diff] [review]
events_in_nursery_merge_8.diff

Review of attachment 8700424 [details] [diff] [review]:
-----------------------------------------------------------------

This approach seems fine to me, with one caveat: have you measured the performance to see if this is actually faster in practice on event-heavy workloads?

::: js/public/TracingAPI.h
@@ +302,5 @@
>  extern JS_PUBLIC_API(void)
>  JS_CallObjectTracer(JSTracer* trc, JS::Heap<JSObject*>* objp, const char* name);
>  
>  extern JS_PUBLIC_API(void)
> +JS_CallUnsafeObjectTracer(JSTracer* trc, JSObject** objp, const char* name);

This appears to be idential to JS_CallUnbarrieredObjectTracer. Why do we need the duplication?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Because I wasn't aware of JS_CallUnbarrieredObjectTracer ;)

And measuring this... that is somewhat hard unfortunately. What I'm hoping this to lead is fewer major GCs. Something we could detect in telemetry level.

Comment 26

[Andrew McCreight [:mccr8]]

Comment on attachment 8700424 [details] [diff] [review]
events_in_nursery_merge_8.diff

Review of attachment 8700424 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

Please file a followup for the touch event stuff.

::: dom/base/nsWrapperCache.cpp
@@ +41,5 @@
> +  mWrapper = aWrapper;
> +  UnsetWrapperFlags(kWrapperFlagsMask & ~WRAPPER_IS_NOT_DOM_BINDING);
> +
> +  if (aWrapper && !JS::ObjectIsTenured(aWrapper)) {
> +    CycleCollectedJSRuntime::Get()->NurseryWrapperAdded(this);

Is hitting TLS here going to make Boris sad? :P

::: dom/bindings/parser/WebIDL.py
@@ +1528,5 @@
>  
> +    def isProbablyShortLivingObject(self):
> +        current = self
> +        while current:
> +            if current.getExtendedAttribute("ProbablyShortLivingObject"):

Please add a comment to https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings about this attribute.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +1055,5 @@
> +}
> +
> +void
> +CycleCollectedJSRuntime::NurseryWrapperPreserved(JSObject* aWrapper)
> +{

Maybe check that aWrapper is tenured here?

@@ +1056,5 @@
> +
> +void
> +CycleCollectedJSRuntime::NurseryWrapperPreserved(JSObject* aWrapper)
> +{
> +  mozilla::ThreadsafeAutoSafeJSContext cx;

Why does this have to be threadsafe?

::: xpcom/base/CycleCollectedJSRuntime.h
@@ +371,5 @@
> +
> +  static const size_t kSegmentSize = 512;
> +  SegmentedVector<nsWrapperCache*, kSegmentSize, InfallibleAllocPolicy>
> +    mNurseryObjects;
> +  SegmentedVector<nsAutoPtr<JS::PersistentRooted<JSObject*>>, kSegmentSize,

Does this really need the nsAutoPtr? The indirection seems unfortunate. Maybe ask froydnj or something if there's a way to do it.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] (PTO-ish) from comment #26)
> > +  if (aWrapper && !JS::ObjectIsTenured(aWrapper)) {
> > +    CycleCollectedJSRuntime::Get()->NurseryWrapperAdded(this);
> 
> Is hitting TLS here going to make Boris sad? :P
Hmm, well, we're about to hit TLS just after this too when doing the addref which wrapper has.
 
> > +    def isProbablyShortLivingObject(self):
> > +        current = self
> > +        while current:
> > +            if current.getExtendedAttribute("ProbablyShortLivingObject"):
> 
> Please add a comment to
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings about this
> attribute.
Sure, though, I'll wait for couple of days to see if this all sticks in.

> > +CycleCollectedJSRuntime::NurseryWrapperPreserved(JSObject* aWrapper)
> > +{
> 
> Maybe check that aWrapper is tenured here?
Hmm, I don't see what would guarantee it is tenured at that point. Anyone can just preserve wrapper on Gecko side, and
JS engine doesn't know about it.


> > +CycleCollectedJSRuntime::NurseryWrapperPreserved(JSObject* aWrapper)
> > +{
> > +  mozilla::ThreadsafeAutoSafeJSContext cx;
> 
> Why does this have to be threadsafe?
workers.


> > +  static const size_t kSegmentSize = 512;
> > +  SegmentedVector<nsWrapperCache*, kSegmentSize, InfallibleAllocPolicy>
> > +    mNurseryObjects;
> > +  SegmentedVector<nsAutoPtr<JS::PersistentRooted<JSObject*>>, kSegmentSize,
> 
> Does this really need the nsAutoPtr? The indirection seems unfortunate.
Oh, good point. I'll test if it works without nsAutoPtr.
In general that array is used very rarely, since short living objects tend to not have expando properties.

Comment 28

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #27)
> > Maybe check that aWrapper is tenured here?
> Hmm, I don't see what would guarantee it is tenured at that point. Anyone
> can just preserve wrapper on Gecko side, and
> JS engine doesn't know about it.

Oops, right. How about a DEBUG-only check that things are tenured in that array in CycleCollectedJSRuntime::JSObjectsTenured()?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

That I could do sure.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Attachment: events_in_nursery_merge_10.diff

There is still something odd happening occasionally during worker shutdown if an Event has preserved wrapper. Almost like we wouldn't be doing tenuring during cx destroy gc. Investigating.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44652990eaa2

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

(and I think I know what the issue, missing to clear a pointer, but may not have time to test a fix today.)

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Looks like the crash I saw is about PersistentRooted use. One really needs to initialize it with JSRuntime, not with JSContext. (we should probably stop supporting passing cx as param)
Thanks to Terrence helping with that.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Attachment: events_in_nursery_merge_11.diff

Pass JS_GetRuntime(cx), not just cx to PersistentRooted ctor.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3475d6cdf93d

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Attachment: events_in_nursery_merge_12.diff

additions to annoations.js so that static analysis passes, but I'll try still a simpler version

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Ok, the simpler approach didn't work.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d55a6e4e98e

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for causing 980 hazards in the b2g desktop hazard build: https://treeherder.mozilla.org/logviewer.html#?job_id=19124294&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/081379edf003

Comment 40

[Steve Fink [:sfink] [:s:]]

(In reply to Wes Kocher (:KWierso) (On vacation until Dec 28) from comment #39)
> I had to back this out for causing 980 hazards in the b2g desktop hazard
> build:
> https://treeherder.mozilla.org/logviewer.html#?job_id=19124294&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/081379edf003

Ugh. Olli: sorry, it looks like moveObjectToTenured returns a size_t, which means the annotation only works on 64-bit. You'll need to change

+    "uint64 js::TenuringTracer::moveObjectToTenured(JSObject*, JSObject*, int32)" : true,

to add both

+    "uint64 js::TenuringTracer::moveObjectToTenured(JSObject*, JSObject*, int32)" : true,
+    "uint32 js::TenuringTracer::moveObjectToTenured(JSObject*, JSObject*, int32)" : true,

(the int32 parameter is still int32. It's really an 'AllocKind' enum.)

Comment 41

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/22f51211915b

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Ah, tryserver was timing out (known orange) on b2g hazards, so I relied on 64bit linux.


Thanks sfink, I'll try that.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Attachment: events_in_nursery_merge_14.diff

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ece17eeb83de

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ece17eeb83de