Track expected contents of stack type sets in compiler constraints

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla27
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 816116 [details] [diff] [review]
patch

Right now the CompilerConstraintList accumulated during IonBuilder tracks dependencies on heap type sets, but not stack type sets (which are all frozen at once after the IonBuilder finishes).  With bug 785905 stack type sets are free to mutate during compilation and when finishing it would be good to immediately invalidate the compilation if any of the stack type sets for the original or inlined scripts changed in the interim.

The attached patch does this by having the builder snapshot the expected contents of the stack type sets when it first runs, and checks the snapshot for any changes when finishing, invalidating the compilation if necessary.  The compiler is allowed to add new types to the snapshot if it wants to reflect any additional types it can tolerate being read or passed in at specific points.
Attachment #816116 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Assignee: nobody → bhackett1024
(Assignee)

Updated

5 years ago
Blocks: 925194
Comment on attachment 816116 [details] [diff] [review]
patch

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

::: js/src/jit/MIR.cpp
@@ +2924,4 @@
>  
>      types::TemporaryTypeSet *types = obj->resultTypeSet();
>      if (!types || types->unknownObject()) {
> +        observed->addType(types::Type::AnyObjectType(), alloc);

Shouldn't we check the return value of the addType calls in this function like the addType calls in IonBuilder?
Attachment #816116 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 3

5 years ago
Followup to fix some jit-test failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/755ecb4d6e2c
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/27921f21cddf because the followup still had some shell failures (https://tbpl.mozilla.org/php/getParsedLog.php?id=29099418&tree=Mozilla-Inbound) and meantime lots more showed up, https://tbpl.mozilla.org/php/getParsedLog.php?id=29099706&tree=Mozilla-Inbound in nearly every debug test suite, perhaps the same thing or perhaps not as a near-silent SEGV in every ASan run, https://tbpl.mozilla.org/php/getParsedLog.php?id=29099377&tree=Mozilla-Inbound
(Assignee)

Updated

5 years ago
Depends on: 927544
Blocks: 928307
https://hg.mozilla.org/mozilla-central/rev/81b505e9a435
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 7

5 years ago
Bullet workloads 1-4 seem to have regressed by 25-33% when this landed, none of the other 3 patches in that change list look suspicious. Could this have caused it?

http://arewefastyet.com/#machine=11&view=breakdown&suite=asmjs-apps

Updated

5 years ago
Duplicate of this bug: 925194
Depends on: 930327
No longer depends on: 930327
Depends on: 951497
You need to log in before you can comment on or make changes to this bug.