Switch to using ggc + zeal=7 for dynamic rooting analsyis

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: sfink, Assigned: sfink)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open)

Attachments

(4 attachments, 4 obsolete attachments)

Assignee

Description

6 years ago
Switch to using the ggc build with zeal=7 for the dynamic rooting analysis, and switch both builds over to better match the build flags for the browser.
Assignee

Comment 2

6 years ago
Terrence - I finally managed to get a build out of my slave today, at least for the ggc variant. Sadly, it failed a bunch of tests. Do these failures look familiar? Do I still have the configure flags wrong?

    --baseline-eager --no-ti --no-fpu jit-test/tests/basic/testBug756919.js
    --baseline-eager --no-ti --no-fpu jit-test/tests/basic/testBug840012.js
    jit-test/tests/parallel/scatter-6.js
    --ion-eager jit-test/tests/parallel/scatter-6.js
    --ion-eager --ion-check-range-analysis jit-test/tests/parallel/scatter-6.js
    jit-test/tests/parallel/scatter-7.js
    --baseline-eager jit-test/tests/parallel/scatter-7.js
    --ion-eager jit-test/tests/parallel/scatter-7.js
    --baseline-eager jit-test/tests/parallel/scatter-6.js
    jit-test/tests/parallel/scatter-8.js
    --baseline-eager jit-test/tests/parallel/scatter-8.js
    --ion-eager --ion-check-range-analysis jit-test/tests/parallel/scatter-7.js
    --ion-eager jit-test/tests/parallel/scatter-8.js
    --ion-eager --ion-check-range-analysis jit-test/tests/parallel/scatter-8.js
Assignee

Comment 3

6 years ago
Assignee

Comment 4

6 years ago
Posted patch Cannot AddNamedRoot a nullptr (obsolete) — Splinter Review
The rootanalysis build also made it through, and immediately found a problem in a jsapi-tests test. testChromeBuffer was adding a name root via JS_AddNamedObjectRoot while the object was still null, and the heightened zeal level happened to make it GC during that time.

I probably shouldn't tack this onto this bug.
Attachment #817627 - Flags: review?(terrence)
Comment on attachment 817627 [details] [diff] [review]
Cannot AddNamedRoot a nullptr

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

r=me

Works for me.
Attachment #817627 - Flags: review?(terrence) → review+
Comment on attachment 817522 [details] [diff] [review]
Switch to using ggc + zeal=7 for dynamic rooting analsyis

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

r=me, assuming splinter is being busted.

::: scripts/spidermonkey_builds/rootanalysis
@@ +1,2 @@
>  --enable-optimize
>  --enable-debug

I guess splinter is messing up by not showing any changes here? The right side here should probably have the same set of flags as "generational" now. GGC at least is definitely required for zeal mode 7.
Attachment #817522 - Flags: review?(terrence) → review+
Assignee

Comment 7

6 years ago
(In reply to Terrence Cole [:terrence] from comment #6)
> Comment on attachment 817522 [details] [diff] [review]
> Switch to using ggc + zeal=7 for dynamic rooting analsyis
> 
> Review of attachment 817522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, assuming splinter is being busted.
> 
> ::: scripts/spidermonkey_builds/rootanalysis
> @@ +1,2 @@
> >  --enable-optimize
> >  --enable-debug
> 
> I guess splinter is messing up by not showing any changes here? The right
> side here should probably have the same set of flags as "generational" now.
> GGC at least is definitely required for zeal mode 7.

Yes, splinter somehow got confused. The raw diff has the right stuff. But more to the point, the two files are identical now (rootanalysis and generational). The only difference is in spidermonkey.sh, which sets JS_GC_ZEAL=7 for the rootanalysis build.
Assignee

Comment 9

6 years ago
Attachment #818073 - Flags: review?(terrence)
Assignee

Updated

6 years ago
Whiteboard: [leave open
Depends on: 927663
Assignee

Comment 11

6 years ago
Comment on attachment 818073 [details] [diff] [review]
More jsapi-tests that add null roots

Bug 927633 obsoletes these null root fixes.
Attachment #818073 - Attachment is obsolete: true
Attachment #818073 - Flags: review?(terrence)
Assignee

Updated

6 years ago
Attachment #817627 - Attachment is obsolete: true
Assignee

Comment 12

6 years ago
Comment on attachment 817525 [details]
ggc build log with jit-test failures

This log is for a bug that was fixed independently.
Attachment #817525 - Attachment is obsolete: true
Assignee

Comment 13

6 years ago
Latest batch of failures for new SM(r):

FAILURES:
    --baseline-eager --no-ti --no-fpu basic/metadata-hook.js
    --baseline-eager --no-ti --no-fpu debug/Object-identity-01.js
    --baseline-eager --no-ti --no-fpu debug/Object-identity-03.js
    --baseline-eager debug/Object-unwrap-02.js
    --debugjit --baseline-eager --no-ti --no-fpu debug/breakpoint-gc-03.js
    --ion-eager debug/gc-04.js
    --ion-eager --ion-check-range-analysis debug/gc-04.js
    --baseline-eager debug/gc-04.js
TIMEOUTS:
    basic/bug623859.js
    --ion-eager basic/bug623859.js
    --ion-eager --ion-check-range-analysis basic/bug623859.js
    --baseline-eager basic/bug623859.js
    --baseline-eager --no-ti --no-fpu basic/bug623859.js
    --no-baseline --no-ion basic/bug623859.js
    --no-baseline --no-ion --no-ti basic/bug623859.js
The timeouts are almost certainly related to bug 928056.
Assignee

Updated

6 years ago
Depends on: 929151
I'm getting the same set of failures locally still. Jon, do you have time to take a look at this?
Flags: needinfo?(jcoppeard)
Assignee

Comment 16

6 years ago
The current set of failures on the slave is:

    --ion-eager debug/Script-isInCatchScope.js
    --ion-eager --ion-check-range-analysis debug/Script-isInCatchScope.js
    --ion-eager debug/gc-03.js
    --ion-eager --ion-check-range-analysis debug/gc-03.js
TIMEOUTS:
    basic/bug623859.js
    --ion-eager basic/bug623859.js
    --ion-eager --ion-check-range-analysis basic/bug623859.js
    --baseline-eager basic/bug623859.js
    --baseline-eager --no-ti --no-fpu basic/bug623859.js
    --no-baseline --no-ion basic/bug623859.js
    --no-baseline --no-ion --no-ti basic/bug623859.js

That's really only 3:

    --ion-eager debug/Script-isInCatchScope.js
    --ion-eager debug/gc-03.js
    (timeout) basic/bug623859.js

The first is due to using an uninitialized Value in Debugger::fireExceptionUnwind, and I have a patch. The second is some weird thing where a Debugger.Object is getting detached from the debuggee object it is describing (the D.O. is *not* getting collected, as I first thought; it still exists and has the correct value even after an undefined value is retrieved.) It's probably a matter of the 'objects' WeakMap key moving. I haven't looked at the third.

And I don't know why this set is mostly disjoint from the previous set. Only the timeout is the same. Terrence, what do you get on your desktop?
The |--ion-eager debug/gc-04.js| failure appears to me to be the same underlying issue as 03.
The timeout in basic/bug623859.js is caused by constructing an Iterator for a large typed array.  The case can be reduced to:

  Iterator(Float32Array(4000018));

I don't really understand how iterators work, but I think that typed arrays are supposed to have custom iterator support.  Anyway, that doesn't happen and the array is treated like a regular object.  The iterator constructor creates a snapshot containing the ID of every property on the object, and then every ID is used to create a new string.  So this GCs four million times with zeal on.
Flags: needinfo?(jcoppeard)
So it turns out that this is just how Iterator() works.

Here's a patch to replace this test case with one that exercises the same error path, but without using iterators and typed arrays.  The code is pretty different today, but I checked that this makes the array object creation fail.

For reference, the original fix and test is here: https://hg.mozilla.org/mozilla-central/rev/6ca3394a3aa2
Attachment #822282 - Flags: review?(terrence)
Comment on attachment 822282 [details] [diff] [review]
Bug927204-timeout

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

Nice! r=me
Attachment #822282 - Flags: review?(terrence) → review+
This is looking much better now.

I'm currently hitting an assertion in testParallelCompile.js. In this test, the parser is allocating and enters refillFreeList at the same time as the foreground thread is in MinorGC. Since MinorGC sets heapState = MinorCollecting, the assertion in refillFreeList that !heapIsBusy() fails. On the other hand, refillFreeList takes a ThreadSafeContext, so should be thread safe. I know that refillFreeList is threadsafe with respect to the background sweeping thread, but I don't think it will be threadsafe with respect to two simultaneous allocations.

Brian, what is the intended behavior here? And how is this going to change with bug 928050?
Flags: needinfo?(bhackett1024)
Posted file gdb_output.txt
And gdb backtraces for the competing threads.
(In reply to Terrence Cole [:terrence] from comment #23)
> This is looking much better now.
> 
> I'm currently hitting an assertion in testParallelCompile.js. In this test,
> the parser is allocating and enters refillFreeList at the same time as the
> foreground thread is in MinorGC. Since MinorGC sets heapState =
> MinorCollecting, the assertion in refillFreeList that !heapIsBusy() fails.
> On the other hand, refillFreeList takes a ThreadSafeContext, so should be
> thread safe. I know that refillFreeList is threadsafe with respect to the
> background sweeping thread, but I don't think it will be threadsafe with
> respect to two simultaneous allocations.
> 
> Brian, what is the intended behavior here? And how is this going to change
> with bug 928050?

This should be fixed by bug 928050.
Flags: needinfo?(bhackett1024)
\o/ Awesome!
Depends on: 928050
Assignee

Updated

6 years ago
Attachment #817522 - Attachment is obsolete: true
Comment on attachment 8337114 [details] [diff] [review]
Switch to using ggc + zeal=7 for dynamic rooting analsyis

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

That looks right.
Attachment #8337114 - Flags: review?(terrence) → review+
You didn't say "http://hg.mozilla.org/build/tools/rev/0c8c2c9bd609"

To which I say "backed out in https://hg.mozilla.org/build/tools/rev/f512d3516e24 because rootanalysis said https://tbpl.mozilla.org/php/getParsedLog.php?id=31013522&tree=Mozilla-Inbound 100% of the time including on previously green pushes."
Blocks: 939993
Depends on: 943867
Assignee

Comment 30

6 years ago
Ok, landing attempt 2. The build appears to be green now: https://tbpl.mozilla.org/?tree=Try&rev=29d97ee8e436
Attachment #8339612 - Flags: review?(bhearsum)
Assignee

Comment 31

6 years ago
To be clear, the patch hasn't changed. We've just greened up the build via fixes in Spidermonkey.
Comment on attachment 8339612 [details] [diff] [review]
Switch to using ggc + zeal=7 for dynamic rooting analsyis

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

rubberstamp+. Fwiw, I'm happy for you to land patches that only touch the spidermonkey scripts without RelEng review. Eg, Terrence's review is enough.
Attachment #8339612 - Flags: review?(bhearsum) → review+
Assignee

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.