Closed Bug 1158344 Opened 9 years ago Closed 9 years ago

IonMonkey: Hoist bounds check outside for loops.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

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: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #8598073 - Flags: review?(hv1989)
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 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?
(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 on attachment 8598073 [details] [review]
Add micro benchmark to the assorted test suite.

See comments on the push request.
Attachment #8598073 - Flags: review?(hv1989)
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)
Attachment #8598073 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/a656694f7aed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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
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 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+
Attachment #8598077 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.