Allocate short lived JS wrappers in the Nursery

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: terrence, Assigned: smaug)

Tracking

(Blocks: 4 bugs)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(5 attachments, 7 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8546928 [details] [diff] [review]
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.
Attachment #8546928 - Flags: review-
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.)
(Assignee)

Comment 2

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 1085597
(Reporter)

Comment 3

4 years ago
(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.
(Reporter)

Comment 4

4 years ago
Created attachment 8553897 [details] [diff] [review]
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
Assignee: nobody → terrence
Attachment #8546928 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
do we end up inheriting JSCLASS_FINALIZE_FROM_NURSERY to other Event interfaces, like
MouseEvent?
(Reporter)

Comment 6

4 years ago
(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. :-(
Could just store the boolean on the IDLInterface and copy from the parent in finish().
(Reporter)

Comment 8

4 years ago
(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.
(Reporter)

Comment 9

4 years ago
Created attachment 8554780 [details] [diff] [review]
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
Attachment #8553897 - Attachment is obsolete: true
Attachment #8554780 - Flags: review?(bzbarsky)
(Reporter)

Comment 10

4 years ago
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.
Attachment #8554780 - Flags: review?(bzbarsky)
(Reporter)

Comment 11

4 years ago
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.
(Reporter)

Comment 12

4 years ago
(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.
(Reporter)

Comment 13

4 years ago
Created attachment 8567321 [details] [diff] [review]
1_impl_weakheap_pointer-v0.diff
(Reporter)

Comment 14

4 years ago
Created attachment 8567322 [details] [diff] [review]
2_allocate_events_in_nursery-v3.diff
Attachment #8554780 - Attachment is obsolete: true
(Reporter)

Comment 15

4 years ago
Created attachment 8567326 [details] [diff] [review]
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.
(Assignee)

Comment 16

4 years ago
MutationRecords should be also short living objects, and there can be plenty of them.
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.
Flags: needinfo?(terrence)
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.
Flags: needinfo?(terrence)
Blocks: 1218972
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
(Assignee)

Comment 20

3 years ago
Created attachment 8697075 [details] [diff] [review]
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
Attachment #8697075 - Flags: feedback?(continuation)
Depends on: 1231964
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.
Attachment #8697075 - Flags: feedback?(continuation) → feedback+
(Assignee)

Comment 22

3 years ago
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.
(Assignee)

Comment 23

3 years ago
Created attachment 8700424 [details] [diff] [review]
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.
Assignee: terrence → bugs
Attachment #8697075 - Attachment is obsolete: true
Attachment #8700424 - Flags: review?(terrence)
Attachment #8700424 - Flags: review?(continuation)
(Reporter)

Comment 24

3 years ago
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?
Attachment #8700424 - Flags: review?(terrence) → review+
(Assignee)

Comment 25

3 years ago
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.
Attachment #8567321 - Attachment is obsolete: true
Attachment #8567322 - Attachment is obsolete: true
Attachment #8567326 - Attachment is obsolete: true
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.
Attachment #8700424 - Flags: review?(continuation) → review+
(Assignee)

Comment 27

3 years ago
(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.
(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()?
(Assignee)

Comment 29

3 years ago
That I could do sure.
(Assignee)

Comment 30

3 years ago
Created attachment 8701205 [details] [diff] [review]
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.
(Assignee)

Comment 32

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

Comment 33

3 years ago
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.
(Assignee)

Comment 34

3 years ago
Created attachment 8702422 [details] [diff] [review]
events_in_nursery_merge_11.diff

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

Comment 36

3 years ago
Created attachment 8702887 [details] [diff] [review]
events_in_nursery_merge_12.diff

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

Comment 37

3 years ago
Ok, the simpler approach didn't work.
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
Flags: needinfo?(bugs)
(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.)
(Assignee)

Comment 42

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


Thanks sfink, I'll try that.
Flags: needinfo?(bugs)
(Assignee)

Comment 43

3 years ago
Created attachment 8703126 [details] [diff] [review]
events_in_nursery_merge_14.diff

Comment 45

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ece17eeb83de
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.