Bug 176586 – Speedometer 2.0: Use keyed algorithm to render lists for Inferno suite

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Shiyu Zhang]

Created attachment 320247 [details]
Use keyed algorithm for inferno

Inferno has two ways of rendering lists: Keyed algorithm and Non keyed algorithm. Choosing which one to use is up to the developer. The guide from https://infernojs.org/docs/guides/benefits/list-rendering recommends that keyed algorithm can be used to minimize the number of patch operations. This performs better when changes between lists are minimal, just like the use case of Speedometer's todo-list.

I applied the patch and rebuild the inferno suite to use keyed algorithm, 4x performance improvement for inferno suite was observed. I find that keys are already used for React framework. Shall we use the keys for inferno suite too?

[Ryosuke Niwa]

I guess the big question is whether most apps written using Inferno end up using the keyd algorithm, and if there is a significant number of web apps written using non-keyd algorithm.

If the non-keyed algorithm is a lot slower, then it's of our interest to test that case even if the majority of Inferno apps did indeed used keyed algorithm because the one of many goals of Speedometer is measure things that are currently slow in the browser.

[Ryosuke Niwa]

In general, though, making test cases in Speedometer run faster is not a great thing to do. The whole point of this benchmark is to capture things that are slow, and encourage browser vendors to make things go fast.

If we just tested the highly optimized code, then we're doing disservice to the Web developer community in that we'd be creating a subset of Web facing features that are so slow that nobody can use it. That would ultimately hurt the Web platform by making it harder to use "correctly", and increasing the amount of knowledge needed to write responsive apps on the Web.

[Addy Osmani]

I wanted to echo Ryosuke's comment about the importance of Speedometer reflecting not what is necessarily best practice, but what is practically used by developers in real-world apps. 

It's likely that if the majority of React developers use a keyed algorithm to render lists that Inferno developers are probably doing something similar but it would be good to verify this as much as we can.

I've reached out to Dominic Gannaway of Inferno for his take on this. We may also be able to validate this a little using HTTP Archive.

[Shiyu Zhang]

I agree that Speedometer should be a workload to measure the real-world performance. I am also curious about whether the majority of Inferno developers would choose keyed algorithm for rendering lists like React developers do. Looking forward to hearing more feedback.

[Addy Osmani]

Coming back to this after consulting with the Inferno author. His take was that a keyed list should be getting used by most Inferno users and has been encouraging a keyed list wherever possible due to state in items/DOM nodes. 

After evaluating whether we could determine using HTTP Archive or other data analysis means whether we could find out what folks are actually doing, there isn't a clean way of accomplishing this due to the obfuscation that comes with minifying JS. 

With this in mind, my recommendation is that we land this patch to use the keyed algorithm based on what limited information we have available that suggests it is what Inferno users in the wild would be using.

[Ryosuke Niwa]

We should probably add both variants given the huge 4x difference in the measured time.

[Addy Osmani]

As we included two variants for one of the Ember cases, I'm fine with multiple variants for the Inferno case too. Shiyu Zhang, is this something you would be able to help with?

[Shiyu Zhang]

Thanks for your information. I am a bit hesitant to implement the keyed algorithm Inferno into Speedometer along with non-keyed. Would it be more reasonable to use one keyed algorithm Inferno in Speedometer instead of including two variants of both keyed and non-keyed?

Keys are highly recommended in React, Preact and any other virtual DOM library/framework when handling items in lists. This is clearly stated in the guide docs of Inferno, React and Preact. React will even give a warning that a key should be provided for list items in the console. Given that we have limited information on the Inferno users' choice in the wild world, would it be more reasonable to assume that most web developers have good will to follow the suggestion to use keyed lists? As the Inferno author also confirmed that a keyed list should be getting used by most Inferno users. If that's the case, it makes more sense to keep just keyed algorithm and removes the non-keyed implementation, doesn't it?    

Besides, as we have observed via Chrome Performance tool, the bottleneck of Inferno case is the horrible large changes in DOM tree and layout thrashing caused by non-keyed algorithm. Other than blaming the web engine for this, maybe it's more reasonable to educate the web developers to avoid codes that causes layout thrashing like non-keyed lists. Thus, including a non-keyed Inferno case in Speedometer seems to have little practical value in guiding browsers to optimize their web engine when the major performance loss lies on the misuse of non-keyed lists.  

Meanwhile, the time of running the benchmark was also enlarged by duplicated Ember and Inferno cases, it would be more than appreciated to have the duplicated ones removed, keep the benchmark clean and neat, and reduce the total time of the benchmark execution.

[Ryosuke Niwa]

Shiyu, with all due respect, you're misunderstanding the intent of Speedometer benchmark. The goal of Speedometer benchmark is to measure real world performance, not the highly optimized content on Web. If there's some web content that's slow, that's exactly what we'd like to test in the benchmark.

(In reply to Shiyu Zhang from comment #8)
> Thanks for your information. I am a bit hesitant to implement the keyed
> algorithm Inferno into Speedometer along with non-keyed. Would it be more
> reasonable to use one keyed algorithm Inferno in Speedometer instead of
> including two variants of both keyed and non-keyed?

No.

> Keys are highly recommended in React, Preact and any other virtual DOM
> library/framework when handling items in lists. This is clearly stated in
> the guide docs of Inferno, React and Preact. React will even give a warning
> that a key should be provided for list items in the console. Given that we
> have limited information on the Inferno users' choice in the wild world,
> would it be more reasonable to assume that most web developers have good
> will to follow the suggestion to use keyed lists?

We do have a very important information: Inferno supports non-keyed algorithm. It means that any real website can end up deploying Inferno with non-keyed algorithm. In fact, Inferno example in TodoMVC currently included in Speedometer 2.0 DID use the non-keyed algorithm!

> As the Inferno author also
> confirmed that a keyed list should be getting used by most Inferno users. If
> that's the case, it makes more sense to keep just keyed algorithm and
> removes the non-keyed implementation, doesn't it?

Absolutely not. The goal of Speedometer isn't to measure the performance of well behaving or even well written Web apps. It is to measure the performance of real Web applications. If there is some pathology that a library/framework user could encounter, then that test case should certainly be included in Speedometer.

> Besides, as we have observed via Chrome Performance tool, the bottleneck of
> Inferno case is the horrible large changes in DOM tree and layout thrashing
> caused by non-keyed algorithm.

Given that this could happen in real life, and it DID happen in real life, it's of utmost interests to us to test non-keyed version of Inferno. In fact, I'd even argue that keyed algorithm is less variable as that's already fast in many browsers.

> Other than blaming the web engine for this,
> maybe it's more reasonable to educate the web developers to avoid codes that
> causes layout thrashing like non-keyed lists. Thus, including a non-keyed
> Inferno case in Speedometer seems to have little practical value in guiding
> browsers to optimize their web engine when the major performance loss lies
> on the misuse of non-keyed lists.

Absolutely not.

> Meanwhile, the time of running the benchmark was also enlarged by duplicated
> Ember and Inferno cases, it would be more than appreciated to have the
> duplicated ones removed, keep the benchmark clean and neat, and reduce the
> total time of the benchmark execution.

First off, the runtime increase caused by Inferno is negligible.

Even if it were, that would be an argument for not including the keyed version of Inferno. If some test case is so slow that it's causing a problem in a benchmark, it's surely going to cause a bad user experience in real life.

[Ryosuke Niwa]

Closing this bug as WontFix for now since there has been no response for almost one entire month yet we need to finalize Speedometer 2.0 at some point.