1363054 - Make type monitor stubs work with unknown objects/values

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Micro benchmark

Currently we attach up to 8 monitor stubs and give up, but this hurts us on polymorphic code because we end up with (relatively slow) DoTypeMonitorFallback calls. If the TypeSet is unknown() or unknownObject() we should just attach a single stub to handle these cases.

I have some patches to fix this. It improves the silly micro-benchmark I'm attaching from 55 ms to 15 ms with --no-ion, and when I load Gmail it reduces the number of DoTypeMonitorFallback calls from > 230,000 to less than 40,000, so it seems pretty effective.

After this we should do something similar for the type update stubs.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove unnecessary addMonitorStubForValue calls

The call and spread-call ICs were calling addMonitorStubForValue twice, so let's remove one of these calls.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove shared stubs code

At some point we used Baseline's GetProp IC in Ion (the shared stubs mechanism), but we no longer do that and we don't have any shared stubs that use type monitoring. So now that the type monitoring code is Baseline-specific again, we can simplify/optimize it a bit.

Comment 3

[Ted Campbell [:tcampbell]]

Comment on attachment 8865474 [details] [diff] [review]
Part 1 - Remove unnecessary addMonitorStubForValue calls

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

Ah, looking at the history, this looks like an accidental merge issue. The patch on bug 836064 didn't quite apply correctly.

Good find.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Pass StackTypeSet* to addMonitorStubForValue

The next patch wants to have access to the StackTypeSet in addMonitorStubForValue - in order to check whether it's unknown() or unknownObject() - so here's some plumbing to do that.

I also added a more-optimized version of TypeScript::Monitor that takes the StackTypeSet* so we don't have to call TypeScript::BytecodeTypes twice.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Attach generalized stubs if unknown/unknownObject

If the TypeSet is unknown() we can now attach a TypeMonitor_AnyValue stub.

If the TypeSet is unknownObject(), we can attach a TypeMonitor_PrimitiveSet stub that works with objects.

Comment 6

[Ted Campbell [:tcampbell]]

Comment on attachment 8865483 [details] [diff] [review]
Part 2 - Remove shared stubs code

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

LGTM

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 8865493 [details] [diff] [review]
Part 3 - Pass StackTypeSet* to addMonitorStubForValue

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

Sure. Makes sense as plumbing.

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 8865499 [details] [diff] [review]
Part 4 - Attach generalized stubs if unknown/unknownObject

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

Makes sense to me.

::: js/src/jit/SharedIC.cpp
@@ +2195,5 @@
> +    if (types->unknown()) {
> +        // The TypeSet got marked as unknown so attach a stub that always
> +        // succeeds.
> +
> +        // Check for existing TypeMonitor_AnyValue stubs.

Can this be an assertion, or do things likes DoCallFallback cause us to run addMonitorStubForValue even with AnyValue stub already in place.

Comment 9

[Jan de Mooij [:jandem]]

Thanks for the quick reviews.

(In reply to Ted Campbell [:tcampbell] from comment #8)
> Can this be an assertion, or do things likes DoCallFallback cause us to run
> addMonitorStubForValue even with AnyValue stub already in place.

Yes, exactly.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10acb957ba60
part 1 - Remove redundant addMonitorStubForValue calls. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2281e684a80
part 2 - Remove some shared stubs code that's no longer needed. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d383642cf05
part 3 - Pass StackTypeSet* to addMonitorStubForValue. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/eade14aad3f7
part 4 - Attach generalized type monitor stubs for unknown/unknownObject type sets. r=tcampbell

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing Marionette's test_crash.py TestCrash.test_crash_content_process on Windows 8 x64 opt with e10s:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d420703275427bacc2585f3fd614d1453a5d444b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbec7ac55b3848e7fc3ddeb15987be6d3eaaf1ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/4940da5cc89ed9fb510cc61cc0d6a7e0e42f3bab
https://hg.mozilla.org/integration/mozilla-inbound/rev/f199620366f4b95ad91d12688cef754fe5e6736e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=eade14aad3f788307874a22826079902ce0b9f4d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log (not so helpful): https://treeherder.mozilla.org/logviewer.html#?job_id=97674547&repo=mozilla-inbound

Comment 12

[Ted Campbell [:tcampbell]]

Taking a look at it, I see that if I run the build artifact before and after that the after is certainly broken. Without running the test or even using -marionette flag, I get problems. This also persists when the JITs are turned off (thus negating this patch). I see spinner on startup and sometimes a hidden window in taskbar that is white with native close buttons.

This failure looks like a compiler problem or existing bug affected by memory/symbol layout.

Comment 13

[Ted Campbell [:tcampbell]]

Took another look at it. I'm suspicious this is just an intermittent that got unlucky with compiler or timing. Builds after a few commits and before bailout don't have the glitchy behavior I saw.

A few experiments in progress:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96f97a36071993185ba4d9f7a3f2116d6207641a  // Rebuild the same tree
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a5795ade97fc5cea3dcc7c81db9fe101dcc3b0  // Rebuild with different unified source bundling
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e37ef615e619f2eac1f03b4de1b52548319d1b  // Run Mn-e10s on a later commit but before the backout

Comment 14

[Jan de Mooij [:jandem]]

Ted, thanks a lot for looking at this!

I agree the failure is pretty weird. I'm going to bisect the patches on Try so we can land the ones that are okay and narrow it down more.

Comment 15

[Ted Campbell [:tcampbell]]

If you are good at windows debugging, take a look at that win64 build on a local machine. It is consistently unable to start without glitches (no flags or tests. Just open with a blank profile). Even with the JITs pref'd off. The first tab fails to render at all.

Comment 16

[Jan de Mooij [:jandem]]

So, getting back to this, it looks like the badness starts with part 3. I'll push parts 1 and 2 since they're strict improvements and will investigate more next week.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2730adba4bc
part 1 - Remove redundant addMonitorStubForValue calls. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/131c75bd410c
part 2 - Remove some shared stubs code that's no longer needed. r=tcampbell

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d2730adba4bc
https://hg.mozilla.org/mozilla-central/rev/131c75bd410c

Comment 19

[Andrew Overholt [:overholt]]

Kannan had some fancytalk reasons why this should be p1.

Comment 20

[Jan de Mooij [:jandem]]

I'm doing some Try pushes now. Removing changes from part 3 until I have a minimal patch that triggers the problem. Part 3 is not very big so hopefully we'll have more information soon.

Comment 21

[Jan de Mooij [:jandem]]

It seems to be an MSVC compiler bug, triggered by the DoTypeMonitorFallback changes. Making small, almost-no-op modifications to that part of the patch makes it green or orange on Try.

I'll see if I can rewrite this code in a way that makes MSVC happy.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d45d197ffb05
part 3 - Pass StackTypeSet* to addMonitorStubForValue. r=tcampbell

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Pulsebot from comment #22)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d45d197ffb05
> part 3 - Pass StackTypeSet* to addMonitorStubForValue. r=tcampbell

Fingers crossed this one will stick. I rewrote DoTypeMonitorFallback a bit in a way that doesn't make Mn-e10s angry on Try. It's pretty weird, I tried some simpler workarounds but they didn't help - sometimes opt builds were green but then debug builds showed the problem...

Comment 24

[Ted Campbell [:tcampbell]]

Strange.. Did you keep any of those broken debug builds? It would be interesting to compare obj files and determine if miscompile is restricted to DoTypeMonitorFallback, or whether there is other fallout that may be pre-existing.

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] from comment #24)
> Strange.. Did you keep any of those broken debug builds? It would be
> interesting to compare obj files and determine if miscompile is restricted
> to DoTypeMonitorFallback, or whether there is other fallout that may be
> pre-existing.

Yeah it's strange only this test fails on Win64 even though this function is pretty hot - could be a compiler bug affecting code generated elsewhere. I may take a closer look at it but I've already spent a lot of time on this and I have a bunch of other bugs on my plate, so I don't know if it's worth it...

Comment 26

[Ted Campbell [:tcampbell]]

I noticed the problem manifesting as content processes on start-up being weird and the Mn test only noticed because it wasn't able to shutdown browser properly. I am suspicious other code might be messed up by the compiler.

Maybe if you could link up the version that causes debug to fail. We can look at some point later. Probably not worth you spending more time on though.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c132697561b0
part 4 - Attach generalized type monitor stubs for unknown/unknownObject type sets. r=tcampbell

Comment 29

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] from comment #26)
> Maybe if you could link up the version that causes debug to fail.

See this Try push for instance: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0f0f5e82972f4eb8e5a53adcc340204d7e8755&group_state=expanded

Comment 30

[Phil Ringnalda (:philor)]

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

Comment 31

[Jan de Mooij [:jandem]]

For posterity, this was at least a 6% improvement on the jslint benchmark on AWFY and there were some other improvements elsewhere.