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

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

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

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.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Attachment: ggc build log with jit-test failures

Comment 4

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

Attachment: Cannot AddNamedRoot a nullptr

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.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 817627 [details] [diff] [review]
Cannot AddNamedRoot a nullptr

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

r=me

Works for me.

Comment 6

[Terrence Cole [:terrence]]

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.

Comment 7

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

(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.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f93601415e1

Comment 9

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

Attachment: More jsapi-tests that add null roots

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f93601415e1

Comment 11

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

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

Bug 927633 obsoletes these null root fixes.

Comment 12

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

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

This log is for a bug that was fixed independently.

Comment 13

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

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

Comment 14

[Terrence Cole [:terrence]]

The timeouts are almost certainly related to bug 928056.

Comment 15

[Terrence Cole [:terrence]]

I'm getting the same set of failures locally still. Jon, do you have time to take a look at this?

Comment 16

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

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?

Comment 17

[Terrence Cole [:terrence]]

The |--ion-eager debug/gc-04.js| failure appears to me to be the same underlying issue as 03.

Comment 18

[Jon Coppeard (:jonco)]

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.

Comment 19

[Jon Coppeard (:jonco)]

Attachment: Bug927204-timeout

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

Comment 20

[Terrence Cole [:terrence]]

Comment on attachment 822282 [details] [diff] [review]
Bug927204-timeout

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

Nice! r=me

Comment 21

[Jon Coppeard (:jonco)]

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

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Terrence Cole [:terrence]]

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?

Comment 24

[Terrence Cole [:terrence]]

Attachment: gdb_output.txt

And gdb backtraces for the competing threads.

Comment 25

[Brian Hackett [Laid off!]]

(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.

Comment 26

[Terrence Cole [:terrence]]

\o/ Awesome!

Comment 27

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

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

Comment 28

[Terrence Cole [:terrence]]

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.

Comment 29

[Phil Ringnalda (:philor)]

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."

Comment 30

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

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

Ok, landing attempt 2. The build appears to be green now: https://tbpl.mozilla.org/?tree=Try&rev=29d97ee8e436

Comment 31

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

To be clear, the patch hasn't changed. We've just greened up the build via fixes in Spidermonkey.

Comment 32

[[e-mail] (:bhearsum)]

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.

Comment 33

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

http://hg.mozilla.org/build/tools/rev/f3543a0a37b4
http://hg.mozilla.org/build/tools/rev/f512d3516e24 - backed out 0c8c2c9bd609
http://hg.mozilla.org/build/tools/rev/0c8c2c9bd609