Make type monitor stubs work with unknown objects/values

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(5 attachments)

(Assignee)

Description

4 months ago
Created attachment 8865472 [details]
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.
(Assignee)

Comment 1

4 months ago
Created attachment 8865474 [details] [diff] [review]
Part 1 - Remove unnecessary addMonitorStubForValue calls

The call and spread-call ICs were calling addMonitorStubForValue twice, so let's remove one of these calls.
Attachment #8865474 - Flags: review?(tcampbell)
(Assignee)

Comment 2

4 months ago
Created attachment 8865483 [details] [diff] [review]
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.
Attachment #8865483 - Flags: review?(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.
Attachment #8865474 - Flags: review?(tcampbell) → review+
(Assignee)

Comment 4

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

Comment 5

4 months ago
Created attachment 8865499 [details] [diff] [review]
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.
Attachment #8865499 - Flags: review?(tcampbell)
Comment on attachment 8865483 [details] [diff] [review]
Part 2 - Remove shared stubs code

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

LGTM
Attachment #8865483 - Flags: review?(tcampbell) → review+
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.
Attachment #8865493 - Flags: review?(tcampbell) → review+
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.
Attachment #8865499 - Flags: review?(tcampbell) → review+

Updated

4 months ago
Whiteboard: [qf]
(Assignee)

Comment 9

3 months ago
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

3 months ago
Pushed by jandemooij@gmail.com:
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
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
Flags: needinfo?(jdemooij)
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.
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
(Assignee)

Comment 14

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

Comment 16

3 months ago
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

3 months ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Updated

3 months ago
Flags: needinfo?(jdemooij)
Keywords: leave-open
(Assignee)

Updated

3 months ago
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/d2730adba4bc
https://hg.mozilla.org/mozilla-central/rev/131c75bd410c

Updated

3 months ago
Blocks: 1351769
Whiteboard: [qf] → [qf:p1]
Kannan had some fancytalk reasons why this should be p1.
(Assignee)

Comment 20

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

Comment 21

3 months ago
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

3 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d45d197ffb05
part 3 - Pass StackTypeSet* to addMonitorStubForValue. r=tcampbell
(Assignee)

Comment 23

3 months ago
(In reply to Pulsebot from comment #22)
> Pushed by jandemooij@gmail.com:
> 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...
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.
(Assignee)

Comment 25

3 months ago
(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...
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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d45d197ffb05

Comment 28

3 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c132697561b0
part 4 - Attach generalized type monitor stubs for unknown/unknownObject type sets. r=tcampbell
(Assignee)

Comment 29

3 months ago
(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
Flags: needinfo?(jdemooij)
Keywords: leave-open

Comment 30

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c132697561b0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 31

3 months ago
For posterity, this was at least a 6% improvement on the jslint benchmark on AWFY and there were some other improvements elsewhere.
You need to log in before you can comment on or make changes to this bug.