As explained in Bug 1129313 comment 23, currently the Next function implemented for Array / Map iterators are slow because Scalar Replacement see multiple return paths, including one which is unknown. In practice, this first branch is removed by GVN, and Scalar Replacement could run successfully iff we it was aware of the types of the slots. Currently, the only reason why Scalar Replacement is so early in the pipeline, is that the transformation ignores the type of the stored value, and ignores the type of the loaded values. In order to safely do so, we would have to ensure that properties/elements are properly stored/loaded.

This is a simple patch which moves Scalar Replacement after GVN, and retrigger all other optimizations which might benefit from object removal if we have hits after the Scalar Replacement. I will run some benchmarks and ask for review if this naive approach is sufficient and if it does not cause any obvious regressions. This modification is enough to ensure that we are no longer allocating any object for Array[@@iterator]().next() function calls.

Should bug 1165569 be closed as a duplicate of this bug?

(In reply to André Bargull from comment #2) > Should bug 1165569 be closed as a duplicate of this bug? As there is no conditional branch, I do not think this bug will change anything. Also, I do not think that destructuring cause us to allocate any objects, so this bug is unlikely to change the performance of Bug 1165569. This bug would remove the object allocation, iff the object got created in the same scope, or in an inlined function. This patch enables scalar replacement under condition where GVN is able to remove paths which can be proved to be non-taken, and blocking scalar replacement optimizations.

(In reply to Nicolas B. Pierron [:nbp] from comment #3) > Also, I do not think that destructuring cause us to allocate any > objects, so this bug is unlikely to change the performance of Bug 1165569. Array destructuring uses Array[@@iterator]() under the hood, therefore I thought this part from comment #1 also applies to array destructuring: (In reply to Nicolas B. Pierron [:nbp] from comment #1) > This modification is enough to ensure that we are no longer allocating any > object for Array[@@iterator]().next() function calls.

(In reply to André Bargull from comment #4) > (In reply to Nicolas B. Pierron [:nbp] from comment #3) > > Also, I do not think that destructuring cause us to allocate any > > objects, so this bug is unlikely to change the performance of Bug 1165569. > > Array destructuring uses Array[@@iterator]() under the hood, therefore I > thought this part from comment #1 also applies to array destructuring: Oh, I didn't know. Then yes, this will help, then I have no idea if this is the only source of issues, but this should remove the object allocation returned by the next functions.

Comment on attachment 8607083 [details] [diff] [review] Move Scalar Replacement after GVN. The following results are the benchmarks results I got on a loaded computer. I double checked Richards and Mandreel results with IONFLAGS=escape, but no spew indicated that this patch changed anything. Other benchmarks are under the noise level of 2% associated with this number of runs. Runs: 19 19 MandreelLatency: 42510 41014 delta: -3.52% Richards: 32152 34548 delta: 7.45% I will add a for-in benchmarks on AWFY, such as we can see how we compare to others, and follow regressions.

Checking on a fork of misc-basic-array.js which is using for-of and for-in, the unboxed-plain objects kicks-in and prevent escape analysis to optimize anything. Also, the shortcut paths added for unboxed plain objects does not report any message on the Escape spew, which means that Scalar Replacement was not reporting that we were not able to optimized the allocation. Default with --no-unboxed-objects tip qtip tip qtip basic-array ~59ms ~59ms ~59ms ~59ms basic-array-forin ~11.4s ~11.3s ~10.7s ~11.3s basic-array-forof ~3.3s ~3.4s ~560ms ~340ms I will open another bug to support unboxed object in the Object state. I will also open a bug to figure out why basic-array-forof is slower with unboxed plain objects. For-in is not optimized, as it does not rely on @@iterator, and has some specific iterator instructions as part of the jit, which apparently are way slower.

Comment on attachment 8608085 [details] AreWeFastYet PullRequest #72 I pushed a new commit which remove for-in tests, as this is apparently known to be slow on array. And I reduced the number of iterations by 10, such that v8 can hopefully fit under an upper-bound of 1s.

Comment on attachment 8607083 [details] [diff] [review] Move Scalar Replacement after GVN. Review of attachment 8607083 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Nice improvement! ::: js/src/jit/Ion.cpp @@ +1264,4 @@ > > + size_t doRepeatOptimizations = 0; > + repeatOptimizations: > + doRepeatOptimizations++; MOZ_ASSERT(doRepeatOptimizations <= 2); ?

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b772e603c42f

nbp backed this out - also landed on m-c as https://hg.mozilla.org/mozilla-central/rev/b772e603c42f

(In reply to Nicolas B. Pierron [:nbp] from comment #0) > As explained in Bug 1129313 comment 23, currently the Next function > implemented for Array / Map iterators are slow because Scalar Replacement > see multiple return paths, including one which is unknown. This approach is no longer needed since Branch Pruning optimization provides the same kind of optimization.