Closed
Bug 1322724
Opened 8 years ago
Closed 8 years ago
Regression caused by the splitting of Ionbuilder
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
The splitting caused some regressions. Tracking in this bug:
https://treeherder.mozilla.org/perf.html#/alerts?id=4482
https://treeherder.mozilla.org/perf.html#/alerts?id=4483
https://treeherder.mozilla.org/perf.html#/alerts?id=4480
https://treeherder.mozilla.org/perf.html#/alerts?status=0&framework=5&filter=1322724&hideTo=1&page=1
Assignee | ||
Comment 2•8 years ago
|
||
Disable this optimization I added during the ionbuilder split. It seems to introduce some regressions. I think it is related to adding a block in a bad place that interrupts the flow of the true branch. I'll open follow-up bug to try to enable it again.
Assignee: nobody → hv1989
Attachment #8817679 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•8 years ago
|
||
Fix for this.
1) We didn't set the hit counts on the extra block added in visitTest to allow for optimizing the false branch. Which resulted in pruning not taking place. This was the bug I encountered when investigating.
2) Ditto for table switch
3) Improvement on branches where a second block was added on the false branch and make sure it gets added just before the false branch. Instead of just after the true branch.
Attachment #8817679 -
Attachment is obsolete: true
Attachment #8817679 -
Flags: review?(jdemooij)
Attachment #8817703 -
Flags: review?(jdemooij)
Comment 4•8 years ago
|
||
Comment on attachment 8817703 [details] [diff] [review]
Patch
Review of attachment 8817703 [details] [diff] [review]:
-----------------------------------------------------------------
Good finds.
::: js/src/jit/IonBuilder.cpp
@@ +1513,5 @@
> if (!setCurrentAndSpecializePhis(mblock))
> return false;
> graph().addBlock(mblock);
>
> +
Nit: rm second blank line.
Attachment #8817703 -
Flags: review?(jdemooij) → review+
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b877875abb3a
IonMonkey - Add the hit count information on the extra false branch blocks, r=jandem
Assignee | ||
Updated•8 years ago
|
Priority: -- → P1
Comment 6•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 7•8 years ago
|
||
this looks to have given is a perf boost on android perf tests:
== Change summary for alert #4567 (as of December 20 2016 21:47 UTC) ==
Improvements:
3% local-blank summary android-4-4-armv7-api15 opt 1287.25 -> 1253.89
2% remote-blank summary android-4-4-armv7-api15 opt 1307.07 -> 1277.98
2% local-blank summary android-4-4-armv7-api15 opt 1285.41 -> 1258.49
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4567
I see wins in many of our other tests, that is just the alerts we have :)
You need to log in
before you can comment on or make changes to this bug.
Description
•