Open Bug 1317690 Opened 8 years ago Updated 11 hours ago

Support Generators in IonMonkey

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox53 --- affected

People

(Reporter: till, Unassigned, NeedInfo)

References

(Blocks 13 open bugs)

Details

With async/await, Generator performance becomes more important, so at some point we should support them in IonMonkey.

I don't know *how* hard this is, just that it is hard. I also don't really know the right priority for this: the public web won't be using native async/await or Generators for some time, but platform code and addons certainly will.
It would be interesting to check how we actually perform here, compared to other engines. Our generators were a bit faster than in V8 when I added Baseline support, but Turbofan is supposed to handle generators so that probably changed.
Priority: -- → P3
Blocks: es6perf, ares-6
Blocks: 1393712
Blocks: 1521435
Blocks: 1541671
Type: defect → enhancement
Blocks: 1565549
No longer blocks: 1565549

Hi,

We are using a generator heavy approach in a new web component. Turns out it runs really slowly in Firefox. Found this jsperf (https://jsperf.com/generator-overhead-2) and tried it on Chrome, Safari and FF on my machine. Safari was roughly twice as fast as FF, Chrome seven times faster. Any hope of this improving soon? Would be great!

Regards,
Johan Isaksson

Any hope for this issue to be resolved? Is it on the radar at least (included in the roadmap)?

(In reply to Nickolay Platonov from comment #4)

Any hope for this issue to be resolved? Is it on the radar at least (included in the roadmap)?

From what I recall this was in the pipeline for December 2019, however, we since decided to simplify a lot SpiderMonkey to reduce the security risks related to IonBuilder usage. There is a project which is currently investigating the simplification of IonBuilder, and maybe replace it by WarpBuilder (Bug 1613592).

While this is not directly related, this is a higher priority at the moment.
I will raise this topic to the team, to see if we should raise the priority of generators in IonMonkey.

Thanks for the update! Yes, please draw some attention to this issue. We wrote a "modern" system, that uses generators a lot, and discovered the performance in FF is very bad (#1565549). In the same time its so powerful feature, arguably a best addition to JS spec in its history.

Yeah we expect this to be significantly easier with WarpBuilder in the future. Note though that Ion/Warp-compiling them is just part of it, generators also require more environment objects right now, and that could be a bigger issue than them not running in Ion.

Severity: normal → S3
Blocks: 1869348
Blocks: 1870552
Blocks: 1874632
Blocks: 1850079

Hi,

Any hope of generators getting optimized any time soon? We have for the last 5 years had to recommend our customers to pick another browser when using our components, ideally they should be able to use Firefox too.

Adding my voice here too, this is becoming a critical defect for us. Seems odd that FF is so far behind Chrome and the rest for 8+ years.

Do you offer a way for us sponsor a fast-tracking this implementation?

Is there a public website we could use to reproduce the performance problem you're seeing? If not, maybe you could share a performance profile from the Firefox profiler?

Flags: needinfo?(mats)
Flags: needinfo?(johan)

See a benchmark here for a example: https://bugzilla.mozilla.org/show_bug.cgi?id=1565549

@Jan de Mooij - Any idea if we could somehow sponsor the fast-track of this bug?

Flags: needinfo?(mats)

(In reply to Mats from comment #13)

@Jan de Mooij - Any idea if we could somehow sponsor the fast-track of this bug?

At this point it would be most useful for us to have access to a website that demonstrates the performance problem. That would help us determine if there's low-hanging fruit, other performance issues, parts we could prioritize, etc.

(In reply to Mats from comment #15)

Isn't this test case enough? https://bugzilla.mozilla.org/show_bug.cgi?id=1565549

The code that's being used on an actual website is likely pretty different from the micro-benchmark linked there. There are multiple parts to optimizing generators and we just want to make sure we're looking at a representative benchmark, to avoid optimizing something that isn't relevant for your framework.

To examine things on real website, please do the following:

  1. Open the https://bryntum.com/products/gantt/examples/bigdataset/ page. (If you'd like to check other examples, see https://bryntum.com/products/gantt/examples/)
  2. In console, type: window.DEBUG = true
  3. Click the "5K Tasks" button in the top toolbar. This will switch the example gantt to a new dataset of 5K tasks.

You should see the following output in console:

For Chrome:

window.DEBUG = true
true
generate: 25.85107421875 ms
Initializing project
Populating project: 887.9541015625 ms
Time to visible: 5474.97509765625 ms
Finalize propagation: 708.40185546875 ms

For Firefox:

window.DEBUG = true
true
generate: 303ms - timer ended
Initializing project
Populating project: 18714ms - timer ended 
Time to visible: 168067ms - timer ended 
Finalize propagation: 21519ms - timer ended

What happens here is:

  1. generate - the example data is generated. Generation uses generators, perhaps that explains the 10x slowdown compared to Chrome, however, this is not a primary metric we are interested in.
  2. Populating project - example data is loaded into "reactive graph". This does not involve generators, but instantiates lots of small objects (like hundreds of thousands of them). 21x slowdown compared to Chrome, would be great to see it optimized to be comparable.
  3. Time to visible - this metric is the topic of this issue. Here the "reactive graph" calculates the final state. 30x slowdown compared to Chrome. Calculations are generators-based. During calculations, it also allocates the edges in the graph (populates lots of Map instances).
  4. Finalize propagation - not related to generators, 30x slowdown.

Hopefully the numbers are self-explanatory, if you need any other information and/or benchmark, please let me know.

Probably one need to optimize the allocation speed too, because the most important metric Time to visible is a combination of generator calls + allocation. In particular, the allocation of Maps, and internal allocations by Maps. I'll open a new ticket for that.

Just opening https://bryntum.com/products/gantt/examples/bigdataset/
Nightly: https://share.firefox.dev/4b9raNf (3s)
Chrome: https://share.firefox.dev/4bb4Umm (1.8s)

Clicking on 5K
Nightly: https://share.firefox.dev/3UQdzVo (12s)
Chrome: https://share.firefox.dev/3WPzMVd (3.5s)


With window.DEBUG = true , I generally get 11s with Firefox. But one time, Firefox took 45s.

Separate issue to optimize the allocations: https://bugzilla.mozilla.org/show_bug.cgi?id=1895667

Thanks, that's helpful.

There's some generator overhead, but it's mostly time spent elsewhere. There are slow GetProp ICs that might benefit from bug 1878158. These are mostly looking up fieldMap and getFieldDefinition properties. There's also some Map overhead (bug 1851662).

(In reply to Jan de Mooij [:jandem] from comment #20)

There are slow GetProp ICs that might benefit from bug 1878158. These are mostly looking up fieldMap and getFieldDefinition properties. There's also some Map overhead (bug 1851662).

To expand on this a bit: the JS code has a lot of JS getter functions. If you could make these non-getter properties it would likely make things faster in all browsers.

Thanks for pointing, we'll try to improve in this regard.

@Jan de Mooij: Just checking in, is this being worked on by anyone? We are facing massive issues in our business due to this issue (and related perf issues above)

So, this isn't being actively looked at at the moment. But what I am going to do is spin the Bryntum specific aspect of this bug out into another bug. This is the wrong bug to be having all this discussion.

I've opened Bug 1898545; we should move discussion of Bryntum specifically there.

Blocks: 1824282
Blocks: 1824279
You need to log in before you can comment on or make changes to this bug.