Closed
Bug 1158344
Opened 9 years ago
Closed 9 years ago
IonMonkey: Hoist bounds check outside for loops.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: nbp, Assigned: nbp)
Details
Attachments
(3 files)
The following pieces of code have some performance differences, caused by the fact the we do not hoist the bounds check in the case of for-loops, as detailed by mraleph[1]. Toby Reif, made another version of this micro-benchmark where the length is manually hoisted[2], and where both loops have the same performance. commas = 0; var i = 0, l = str.length; while (i < l) { if (str.charCodeAt(i) == 44) { commas ++; } i++; } commas = 0; for (var i = 0; i < str.length; i++) { if (str.charCodeAt(i) === 44 /* ',' */) commas++; } We should investigate why the string length is not hoisted when we have a for-loop, and add these benchmarks to AreWeFastYet, to ensure that we do not regress performances in the future. [1] https://twitter.com/mraleph/status/590877047439888384 [2] http://jsperf.com/count-commas-in-strings/7
Assignee | ||
Comment 1•9 years ago
|
||
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #8598073 -
Flags: review?(hv1989)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8598075 -
Flags: review?(sunfish)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8598077 -
Flags: review?(sunfish)
Comment 4•9 years ago
|
||
Comment on attachment 8598075 [details] [diff] [review] part 1 - Use the an instruction which is not a beta node for hoisting bounds checks Review of attachment 8598075 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.cpp @@ +2108,5 @@ > { > // The bounds check's length must be loop invariant. > + MDefinition *length = ins->length(); > + while (length->isBeta()) > + length = length->toBeta()->input(); A loop like this also appears in analyzeLoopIterationCount. Please add a helper function, named something like LookThroughBetaNodes, to do this loop.
Attachment #8598075 -
Flags: review?(sunfish) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8598077 [details] [diff] [review] part 2 - Do not produce upper bounds check if the condition is always verified. Review of attachment 8598077 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.cpp @@ +2172,4 @@ > preLoop->insertBefore(preLoop->lastIns(), lowerCheck); > + > + // Hoist the loop invariant upper bounds checks. > + if (upperTerm != length || upperConstant >= 0) { Can upperConstant be negative here? Does this code handle it correctly if it is?
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #5) > ::: js/src/jit/RangeAnalysis.cpp > @@ +2172,4 @@ > > preLoop->insertBefore(preLoop->lastIns(), lowerCheck); > > + > > + // Hoist the loop invariant upper bounds checks. > > + if (upperTerm != length || upperConstant >= 0) { > > Can upperConstant be negative here? Does this code handle it correctly if it > is? Yes, in fact most of the time it is negative, as the comment which is above mention > upperTerm + upperConstant < boundsLength In the micro benchmark, the LinearSum will set this upperConstant to -1. I think we could even replace the case where both the upperTerm and length are equal by an MBail(BoundCheck).
Comment 7•9 years ago
|
||
Comment on attachment 8598073 [details] [review] Add micro benchmark to the assorted test suite. See comments on the push request.
Attachment #8598073 -
Flags: review?(hv1989)
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8598073 [details] [review] Add micro benchmark to the assorted test suite. Asking again for review, since I pushed a new version of the patch.
Attachment #8598073 -
Flags: review?(hv1989)
Updated•9 years ago
|
Attachment #8598073 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8598075 [details] [diff] [review] part 1 - Use the an instruction which is not a beta node for hoisting bounds checks https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaffa107fb01 https://hg.mozilla.org/integration/mozilla-inbound/rev/a656694f7aed
Attachment #8598075 -
Flags: checkin+
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a656694f7aed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 12•9 years ago
|
||
AWFY detected a regression/improvement on: - slave: Mac OS X 10.10 32-bit (Mac Pro, shell) - mode: Ion Regression(s)/Improvement(s): - misc: basic-arguments: -13.53% (improvement) - misc: bugs-636096-model2d: -1.11% (improvement) - misc: basic-hoist-bounds-check: -9.8% (improvement) Recorded range: - http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2c6a7013693f&tochange=57fc3025cf99 More details: http://arewefastyet.com/regressions/#/regression/1457651
Comment 13•9 years ago
|
||
AWFY detected a regression/improvement on: - slave: Mac OS X 10.10 64-bit (Mac Pro, shell) - mode: Ion Regression(s)/Improvement(s): - ss: unpack-code: -3.77% (improvement) - misc: basic-strcat: -4.63% (improvement) - misc: basic-arguments: -5.88% (improvement) - misc: basic-hoist-bounds-check: -9.94% (improvement) Recorded range: - http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2c6a7013693f&tochange=312707328997 More details: http://arewefastyet.com/regressions/#/regression/1457670
Comment 14•9 years ago
|
||
Comment on attachment 8598077 [details] [diff] [review] part 2 - Do not produce upper bounds check if the condition is always verified. Review of attachment 8598077 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.
Attachment #8598077 -
Flags: review?(sunfish) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8598077 -
Flags: checkin+
You need to log in
before you can comment on or make changes to this bug.
Description
•