Closed
Bug 1455280
Opened 7 years ago
Closed 7 years ago
Remove flow sensitive alias analysis
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox61 | --- | affected |
People
(Reporter: mgaudet, Assigned: mgaudet)
References
Details
Attachments
(1 file, 2 obsolete files)
37.57 KB,
patch
|
mgaudet
:
review+
|
Details | Diff | Splinter Review |
It's been dormant for a year, has zero coverage, and is not being tested right now.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mgaudet
Assignee | ||
Comment 1•7 years ago
|
||
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4005fb8d92fab3c30cd49aed63a29013cb1345be
Attachment #8969302 -
Flags: review?(jdemooij)
Comment 2•7 years ago
|
||
Comment on attachment 8969302 [details] [diff] [review]
Remove FlowAliasAnalysis
Review of attachment 8969302 [details] [diff] [review]:
-----------------------------------------------------------------
\o/ Thanks for doing this!
Maybe as a follow-up we can merge AliasAnalysisShared back into AliasAnalysis? That will also make it easier to optimize the code for the C++ compiler when everything is in a single AliasAnalysis.cpp file. We can also devirtualize the analyze method :)
Attachment #8969302 -
Flags: review?(jdemooij) → review+
Comment 3•7 years ago
|
||
Benjamin, this will probably break AWFY because it removes the alias analysis flag from the shell. We should just remove the flow alias analysis from AWFY.
Matthew, we should probably wait for that to avoid unnecessary AWFY downtime.
Flags: needinfo?(bbouvier)
Comment 4•7 years ago
|
||
Oh the fuzzers also want to know about the --ion-aa shell flag removal.
Maybe we could keep it as a no-op if that makes things easier.
Flags: needinfo?(nth10sd)
Flags: needinfo?(choller)
Comment 5•7 years ago
|
||
Keeping things as a no-op around at least for a while helps bisection of older bugs. Maybe we should keep them around for a while, remove them from all fuzzing configs, then at another point in time, remove a bunch of the no-op flags in one swoop.
Flags: needinfo?(choller)
Pretty much what :decoder said. :)
Flags: needinfo?(nth10sd)
Comment 7•7 years ago
|
||
Opened bug 1456155; it'd be nice to wait for a few hours before this bug here gets landed (see comment there).
Flags: needinfo?(bbouvier)
Comment 8•7 years ago
|
||
Ok so let's just keep this shell flag as a no-op for now, so we don't break fuzzing.
Assignee | ||
Comment 9•7 years ago
|
||
The option is left behind to ease transition for fuzzers.
Assignee | ||
Updated•7 years ago
|
Attachment #8969302 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8970655 [details] [diff] [review]
Remove FlowAliasAnalysis implementation
Carrying Jan's r+
Attachment #8970655 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8970655 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8970663 -
Flags: review+
Comment 12•7 years ago
|
||
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3fc868b50a2
Remove FlowAliasAnalysis implementation r=jandem
Removed from the funfuzz harness in:
https://github.com/MozillaSecurity/funfuzz/commit/300cd149be727a09e4ba43c99bf42deee1261f58
Comment 14•7 years ago
|
||
bugherder |
Assignee | ||
Comment 15•7 years ago
|
||
I'm going to close this, and open a new bug to remove the option.
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•