Regression on misc-basic-array-forof by making IonBuilder::processIterators transitive




JavaScript Engine: JIT
a year ago
a year ago


(Reporter: h4writer, Unassigned)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox54 affected)




a year ago
See bug 1342369. This is about the first regression. Locally it changed score from 25 to 42 (ms).

Comment 1

a year ago
This was backported to aurora. Marking as such.
Blocks: 1342369
status-firefox53: --- → affected
status-firefox54: --- → affected
Depends on: 1333946


a year ago
Flags: needinfo?(shu)

Comment 2

a year ago
I'm not sure how to fix this regression. Bug 1331092 gets rid of the ITERCLOSE trynote, which the transitive processIterators is used for. After that lands, and we also move destructuring code off of trynotes, we can stop using processIterators for ES6-style iterators.

That said, IteratorClose is straight up slow. Bug 1331092 will implement them as try-catches, which I'm sure will have its own share of slowdowns. To improve here (which I don't have time to do in this bug because it'd be involved), we'd need to:

  1. TI knowing that we're iterating with well-behaved iterators that have no weird hooks like "return" or "throw". Builtins like arrays produce such iterators.
  2. Optimize out catch blocks for IteratorClose for such well-behaved iterators, and possible the calls to "next" as well.

Perhaps Ted will be interested? :)
Flags: needinfo?(shu)

Comment 3

a year ago
For extra clarity, IteratorClose is required by spec and we did not implement them before.

Comment 4

a year ago
I'll give it P3. It seems that per spec we need to do some extra work that caused this regression. I'm still leaving this open, since we might want to search for a way to optimize this.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.