1425450 - Per-zone JS holder tables for wrappercached objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Andrew McCreight [:mccr8]]

Zone GCs have to traverse the entire global table of JS holders. We could make per-zone JS holder tables, but this might be tricky to support in general. Wrappercached objects don't add themselves to the holder table until they have an object so it might be easier, and they should constitute the bulk of JS holders.

Tricky cases to think about (that may not apply) are holders that hold things either in multiple zones or that move zones.

Comment 1

[Jon Coppeard (:jonco)]

Thanks for filing this.  The benefits of this change would be that we have to iterate through fewer JS holders for zone GCs, and that we could parallelise this work by breaking it up by zone.

Comment 2

[Andrew McCreight [:mccr8]]

Boris, do you think that wrapper cached things always stay in the same zone (eg any JS they hold onto will always be in the same zone)? I know they can move compartments, but I'm not sure how strong of a guarantee we have that they'll always stay in the same zone.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> do you think that wrapper cached things always stay in the same zone

I don't think so.  For example, if a node is adopted from one toplevel window to another toplevel window in the chrome process, I believe it will change zones.  See SelectZoneGroup in dom/base/nsGlobalWindowOuter.cpp

That said, the place where zones can change is very much chokepointed, and we could remove/readd outselves at that point.  Most simply in nsWrapperCache::SetWrapper.  Or we could hook dom::ReparentWrapper and the SetWrapper call in nsGlobalWindowOuter::SetNewDocument on the codepath where GetWrapperPreserveColor() is not null.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> I don't think so.  For example, if a node is adopted from one toplevel
> window to another toplevel window in the chrome process, I believe it will
> change zones.  See SelectZoneGroup in dom/base/nsGlobalWindowOuter.cpp

Hmm ok. I think there's only a single chrome zone per runtime, but maybe these aren't chrome windows you are talking about? I have a patch to check zone stuff running and I'll see if that hits any asserts.

> That said, the place where zones can change is very much chokepointed, and
> we could remove/readd outselves at that point.  Most simply in
> nsWrapperCache::SetWrapper.  Or we could hook dom::ReparentWrapper and the
> SetWrapper call in nsGlobalWindowOuter::SetNewDocument on the codepath where
> GetWrapperPreserveColor() is not null.

Thanks for the information. I figured there would be some place to do it, I was just hoping to avoid the complexity if possible. :)

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Thread zone info for wrapper cached objects through to CCJSRuntime. WIP.

This passes through the zone information for the wrapper when it is
preserved to the CCJSRuntime, which saves the zone. When the GC traces
the zone, it is checked against the saved zone. The idea is to check
if we need a write barrier for the wrapper to move it from one holder
table to another, plus show the set up needed outside of the runtime
itself.

This patch does not actually create per-zone holder tables. That will
need to be done, and the grey root tracing itself will need to be
changed to take advantage of it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> but maybe these aren't chrome windows you are talking about?

That's correct.  If you have two random content windows in the parent process, they get different zones.  Whether we actually do that, I don't know.

Comment 7

[Jon Coppeard (:jonco)]

Stealing this.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1425450 - Remove dead JSHolderMap entries lazily when the vector is next iterated r?mccr8

Currently, to remove a holder entry from a vector we swap the entry with the last one in the vector and then shrink the vector (and fix up the map). With per-zone vectors we won't have a reference to the vector to get the last entry. One solution would be to store the zone in the entry and look up the vector in a map but I'd like to avoid this if possible because of the space overhead and the extra lookup.

This patch defers clears the entry's pointers when it is removed and actually removes it when the vector is next iterated.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1425450 - Remove unused IsJsHolder methods r?mccr8

Depends on D68518

Comment 10

[Jon Coppeard (:jonco)]

Attachment: Bug 1425450 - Pass zone information through when preserving wrappers r?mccr8

This is bascially the same as your original patch.

Depends on D68519

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Bug 1425450 - Ensure that we call Drop/HoldJSObjects when a preserved wrapper is changed for one in a different zone r?mccr8

So that we can update the holders table correctly we will need to remove the wrapper from its original vector and add it to the one associated with the new zone.

I tried to make SetPreservingWrapper private but there's still a use in layout/style/Rule.cpp that I couldn't see an obvious fix for.

Depends on D68520

Comment 12

[Jon Coppeard (:jonco)]

Attachment: Bug 1425450 - Use a per-zone vector of JS holders where possible r?mccr8

Currently the JS holders table is represented as a map which contains pointers to entries in a SegmentedVector. This patch keeps the single map but use a vector per zone and also has a catch-all vector for where we don't know the zone or the holder can have pointers to more than one zone.

Depends on D68521

Comment 13

[Jon Coppeard (:jonco)]

Attachment: Bug 1425450 - Only trace JS holders in collecting zones r?mccr8

Now that we have some per-zone vectors we can skip tracing those for zones that are not being collected.

Depends on D68522

Comment 14

[Jon Coppeard (:jonco)]

Here is some data I gathered while browsing around to various news sites, giving the proportions of the various categories of holders (multi-zone, single zone non-wrapper, and single-zone wrapper) and the proportion of holders traced during GC. This shows that ~40% of holders are single-zone wrappers, and that these changes reduces the number of holders traced by nearly 20%.

Proportion of multi-zone holders:
  Mean: 45.1% (of 223 samples)
  0.0 - 0.1:  0.0% 
  0.1 - 0.2:  6.3% +++
  0.2 - 0.3: 21.1% ++++++++++
  0.3 - 0.4: 11.2% +++++
  0.4 - 0.5: 17.0% ++++++++
  0.5 - 0.6: 20.2% ++++++++++
  0.6 - 0.7: 19.7% +++++++++
  0.7 - 0.8:  2.7% +
  0.8 - 0.9:  0.4% 
  0.9 - 1.0:  1.3% 

Proportion of single-zone non-wrappers:
  Mean: 15.9% (of 223 samples)
  0.0 - 0.1: 30.0% +++++++++++++++
  0.1 - 0.2: 40.4% ++++++++++++++++++++
  0.2 - 0.3: 22.9% +++++++++++
  0.3 - 0.4:  4.9% ++
  0.4 - 0.5:  0.4% 
  0.5 - 0.6:  0.0% 
  0.6 - 0.7:  0.0% 
  0.7 - 0.8:  1.3% 
  0.8 - 0.9:  0.0% 
  0.9 - 1.0:  0.0% 

Proportion of single-zone wrappers:
  Mean: 39.0% (of 223 samples)
  0.0 - 0.1:  9.9% ++++
  0.1 - 0.2:  3.1% +
  0.2 - 0.3: 25.6% ++++++++++++
  0.3 - 0.4: 20.2% ++++++++++
  0.4 - 0.5:  6.7% +++
  0.5 - 0.6: 10.3% +++++
  0.6 - 0.7: 16.1% ++++++++
  0.7 - 0.8:  7.6% +++
  0.8 - 0.9:  0.4% 
  0.9 - 1.0:  0.0% 

Proportion of holders traced:
  Mean: 81.5% (of 223 samples)
  0.0 - 0.1:  0.0% 
  0.1 - 0.2:  0.0% 
  0.2 - 0.3:  3.1% +
  0.3 - 0.4:  7.6% +++
  0.4 - 0.5:  4.5% ++
  0.5 - 0.6:  3.6% +
  0.6 - 0.7: 10.3% +++++
  0.7 - 0.8: 11.7% +++++
  0.8 - 0.9:  2.2% +
  0.9 - 1.0: 57.0% ++++++++++++++++++++++++++++

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efac091a3ba4
Remove dead JSHolderMap entries lazily when the vector is next iterated r=mccr8
https://hg.mozilla.org/integration/autoland/rev/5da68de67491
Remove unused IsJsHolder methods r=mccr8
https://hg.mozilla.org/integration/autoland/rev/3e4a52d596bb
Pass zone information through when preserving wrappers r=mccr8
https://hg.mozilla.org/integration/autoland/rev/2a00272e72ba
Ensure that we call Drop/HoldJSObjects when a preserved wrapper is changed for one in a different zone r=mccr8
https://hg.mozilla.org/integration/autoland/rev/4238c59af7bb
Use a per-zone vector of JS holders where possible r=mccr8
https://hg.mozilla.org/integration/autoland/rev/c57ac2e125e8
Only trace JS holders in collecting zones r=mccr8

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 6 changesets (bug 1425450) for hazard failure complaining about nsWrapperCacheInlines

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Cdebug%2Chazard-linux64-haz%2Fdebug%2C%28h%29&fromchange=a4a083abf45c61262506b39bff1d97d102e30da3&tochange=bb80f48dfd8d19cd194a30110821e46544e79d9a&selectedJob=296448871

Backout link: https://hg.mozilla.org/integration/autoland/rev/cd97938ad5404ec7936d8747a57e781eab9aad18

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296448871&repo=autoland&lineNumber=83333

[task 2020-04-06T18:02:30.952Z] Running explain to generate ('hazards.txt', 'unnecessary.txt', 'refs.txt')
[task 2020-04-06T18:02:30.952Z] ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' PATH="/builds/worker/fetches/sixgill/usr/bin:${PATH}" SOURCE='/builds/worker/checkouts/gecko' XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" python2.7 /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/explain.py rootingHazards.txt gcFunctions.txt hazards.txt unnecessary.txt refs.txt
[task 2020-04-06T18:02:33.614Z] Wrote explained_hazards.tmp
[task 2020-04-06T18:02:33.614Z] Wrote unnecessary.tmp
[task 2020-04-06T18:02:33.614Z] Wrote refs.tmp
[task 2020-04-06T18:02:33.614Z] Found 1 hazards 197 unsafe references 0 missing
[task 2020-04-06T18:02:33.615Z] Running heapwrites to generate heapWriteHazards.txt
[task 2020-04-06T18:02:33.615Z] ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' PATH="/builds/worker/fetches/sixgill/usr/bin:${PATH}" SOURCE='/builds/worker/checkouts/gecko' XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" /builds/worker/workspace/obj-haz-shell/dist/bin/js /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyzeHeapWrites.js > heapWriteHazards.txt
[task 2020-04-06T18:03:04.287Z] + check_hazards /builds/worker/workspace/analysis
[task 2020-04-06T18:03:04.287Z] + set +e
[task 2020-04-06T18:03:04.287Z] ++ grep -c 'Function.*has unrooted.*live across GC call' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2020-04-06T18:03:04.288Z] + NUM_HAZARDS=1
[task 2020-04-06T18:03:04.289Z] ++ grep -c '^Function.*takes unsafe address of unrooted' /builds/worker/workspace/analysis/refs.txt
[task 2020-04-06T18:03:04.290Z] + NUM_UNSAFE=197
[task 2020-04-06T18:03:04.290Z] ++ grep -c '^Function.* has unnecessary root' /builds/worker/workspace/analysis/unnecessary.txt
[task 2020-04-06T18:03:04.292Z] + NUM_UNNECESSARY=1292
[task 2020-04-06T18:03:04.292Z] ++ grep -c '^Dropped CFG' /builds/worker/workspace/analysis/build_xgill.log
[task 2020-04-06T18:03:04.356Z] + NUM_DROPPED=0
[task 2020-04-06T18:03:04.356Z] ++ perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!' /builds/worker/workspace/analysis/heapWriteHazards.txt
[task 2020-04-06T18:03:04.358Z] + NUM_WRITE_HAZARDS=0
[task 2020-04-06T18:03:04.358Z] ++ grep -c '^Function.*expected hazard.*but none were found' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2020-04-06T18:03:04.359Z] + NUM_MISSING=0
[task 2020-04-06T18:03:04.359Z] + set +x
[task 2020-04-06T18:03:04.359Z] TinderboxPrint: rooting hazards<br/>1
[task 2020-04-06T18:03:04.359Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>197
[task 2020-04-06T18:03:04.359Z] TinderboxPrint: (unnecessary roots)<br/>1292
[task 2020-04-06T18:03:04.359Z] TinderboxPrint: missing expected hazards<br/>0
[task 2020-04-06T18:03:04.359Z] TinderboxPrint: heap write hazards<br/>0
[task 2020-04-06T18:03:04.360Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'aNewWrapper' of type 'JSObject*' live across GC call at dom/base/nsWrapperCacheInlines.h:83
[task 2020-04-06T18:03:04.360Z] TEST-UNEXPECTED-FAIL | hazards | 1 rooting hazards detected
[task 2020-04-06T18:03:04.360Z] TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit "Inspect Task" link for hazard details
[task 2020-04-06T18:03:04.361Z] + onexit

Comment 17

[Jon Coppeard (:jonco)]

The hazard failure is because it thinks nsWrapperCache::ReleaseWrapper can GC due to 'unresolved nsJSContext.CheckForRightParticipant'. This is a virtual method that's only present in debug builds. Steve, is can we can do to make the analysis ignore this?

Function '_ZN14nsWrapperCache25UpdateWrapperForNewGlobalI11nsISupportsEEvPT_P8JSObject$void nsWrapperCache::UpdateWrapperForNewGlobal(nsISupports*, JSObject*) [with T = nsISupports]' has unrooted 'aNewWrapper' of type 'JSObject*' live across GC call '_ZN14nsWrapperCache14ReleaseWrapperEPv$void nsWrapperCache::ReleaseWrapper(void*)' at dom/base/nsWrapperCacheInlines.h:83
    nsWrapperCacheInlines.h:77: Call(1,2, preserving := this*.PreservingWrapper())
    nsWrapperCacheInlines.h:79: Assume(2,3, preserving*, true)
    nsWrapperCacheInlines.h:79: Call(3,4, __temp_2 := this*.GetWrapperPreserveColor())
    nsWrapperCacheInlines.h:79: Call(4,5, __temp_1 := GetObjectZone(__temp_2*))
    nsWrapperCacheInlines.h:80: Call(5,6, __temp_3 := GetObjectZone(aNewWrapper*))
    nsWrapperCacheInlines.h:80: Assume(6,8, (__temp_1* !=p{JS::Zone} __temp_3*), false)
    nsWrapperCacheInlines.h:79: Assign(8,9, zoneChanged := 0)
    nsWrapperCacheInlines.h:82: Assume(9,10, zoneChanged*, true)
    nsWrapperCacheInlines.h:83: Call(10,13, this*.ReleaseWrapper(aScriptObjectHolder*)) [[GC call]]
    nsWrapperCacheInlines.h:88: Call(13,14, this*.SetWrapper(aNewWrapper*))
GC Function: _ZN14nsWrapperCache14ReleaseWrapperEPv$void nsWrapperCache::ReleaseWrapper(void*)
    void mozilla::cyclecollector::DropJSObjectsImpl(void*)
    void mozilla::CycleCollectedJSRuntime::RemoveJSHolder(void*)
    nsScriptObjectTracer.Trace
    void nsJSContext::cycleCollection::Trace(void*, TraceCallbacks*, void*)
    nsJSContext* DowncastCCParticipant(void*) [with T = nsJSContext]
    static T* DowncastCCParticipantImpl<T, true>::Run(void*) [with T = nsJSContext]
    nsJSContext.CheckForRightParticipant
    unresolved nsJSContext.CheckForRightParticipant

Comment 18

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

Ugh. There's no good reason for this to be unresolved. The real fix for that is bug 1531951 or one of the related patches. But that wouldn't fix this problem; there are implementations of that method that end up calling function pointers, and I'm not sure how many things I would need to annotate for that.

One option would be to make the call to CheckForRightParticipant conditional on #if DEBUG && !defined(XGILL_PLUGIN). Or drop an AutoSuppressGCAnalysis in there. Though ReleaseWrapper is pretty broad. I'm not sure how many things you'll need to annotate away.

Comment 19

[Jon Coppeard (:jonco)]

Attachment: Bug 1425450 - Supress rooting analysis warning when removing a JS holder r?mccr8

Currently we get a rooting hazard when nsWrapperCache::UpdateWrapperForNewGlobal calls ReleaseWrapper because the analysis can't see through the virtual method call.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2c02d53b1cc
Remove dead JSHolderMap entries lazily when the vector is next iterated r=mccr8
https://hg.mozilla.org/integration/autoland/rev/9fb011842691
Remove unused IsJsHolder methods r=mccr8
https://hg.mozilla.org/integration/autoland/rev/9a855d95e393
Pass zone information through when preserving wrappers r=mccr8
https://hg.mozilla.org/integration/autoland/rev/9acbb61cea06
Supress rooting analysis warning when removing a JS holder r=mccr8
https://hg.mozilla.org/integration/autoland/rev/994b23b5ec49
Ensure that we call Drop/HoldJSObjects when a preserved wrapper is changed for one in a different zone r=mccr8
https://hg.mozilla.org/integration/autoland/rev/fe4ab7f39aa3
Use a per-zone vector of JS holders where possible r=mccr8
https://hg.mozilla.org/integration/autoland/rev/b7cb030562f1
Only trace JS holders in collecting zones r=mccr8

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a2c02d53b1cc
https://hg.mozilla.org/mozilla-central/rev/9fb011842691
https://hg.mozilla.org/mozilla-central/rev/9a855d95e393
https://hg.mozilla.org/mozilla-central/rev/9acbb61cea06
https://hg.mozilla.org/mozilla-central/rev/994b23b5ec49
https://hg.mozilla.org/mozilla-central/rev/fe4ab7f39aa3
https://hg.mozilla.org/mozilla-central/rev/b7cb030562f1