Bug 175763 – Speedometer: Update to modern Preact version

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mathias Bynens]

Speedometer: Update to modern Preact version

[Mathias Bynens]

Created attachment 318618 [details]
Patch

[Mathias Bynens]

Side note: can we make the style checker ignore `dist` directories under `PerformanceTests/Speedometer`? Currently, I end up having to replace tabs with spaces in the *generated* files before submitting these patches, just to appease the style checks.

[Mathias Bynens]

Created attachment 318629 [details]
Results before and after the patch

[Ryosuke Niwa]

Why did Firefox's score got much worse in this change? 87 -> 78

[Ryosuke Niwa]

(In reply to Mathias Bynens from comment #2)
> Side note: can we make the style checker ignore `dist` directories under
> `PerformanceTests/Speedometer`? Currently, I end up having to replace tabs
> with spaces in the *generated* files before submitting these patches, just
> to appease the style checks.

We could but then commits would fail since we have a pre-commit hook to prevent tab characters from being committed into the repository.

[Mathias Bynens]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Mathias Bynens from comment #2)
> > Side note: can we make the style checker ignore `dist` directories under
> > `PerformanceTests/Speedometer`? Currently, I end up having to replace tabs
> > with spaces in the *generated* files before submitting these patches, just
> > to appease the style checks.
> 
> We could but then commits would fail since we have a pre-commit hook to
> prevent tab characters from being committed into the repository.

I’m guessing the pre-commit hook cannot easily be changed accordingly? :)

[Ryosuke Niwa]

(In reply to Mathias Bynens from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > (In reply to Mathias Bynens from comment #2)
> > > Side note: can we make the style checker ignore `dist` directories under
> > > `PerformanceTests/Speedometer`? Currently, I end up having to replace tabs
> > > with spaces in the *generated* files before submitting these patches, just
> > > to appease the style checks.
> > 
> > We could but then commits would fail since we have a pre-commit hook to
> > prevent tab characters from being committed into the repository.
> 
> I’m guessing the pre-commit hook cannot easily be changed accordingly? :)

You can bypass it if you're a committer but I don't think it's worth the effort. Really, we don't want tab characters in our repository unless absolutely required.

[Ryosuke Niwa]

Comment on attachment 318618 [details]
Patch

With this version, when marking todo items as completed, the items are no longer grayed out with strikethrough.

[Mathias Bynens]

Created attachment 318857 [details]
Patch

[Mathias Bynens]

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 318618 [details]
> Patch
> 
> With this version, when marking todo items as completed, the items are no
> longer grayed out with strikethrough.

Fixed. PTAL.

[Ryosuke Niwa]

So it looks like this change worsens Safari's score by ~8% and improves Chrome's score by ~6% and Firefox's score by ~4%.

In fact, "git diff 8.2.1 7.1.0 src/" in preact repository shows suspicious changes like https://github.com/developit/preact/commit/3f38738eb300a550a51088e3f6992c92685e89a5

Since we're trying to estimate how real world apps behaves, not necessarily trying to measure how specific set of libraries and frameworks behave, it's not great to have these V8 specific optimizations in libraries & frameworks we include in Speedometer since it introduces a bias in our estimation.

[Addy Osmani]

> Since we're trying to estimate how real world apps behaves, not necessarily trying to measure how specific set of libraries and frameworks behave, it's not great to have these V8 specific optimizations in libraries & frameworks we include in Speedometer since it introduces a bias in our estimation.

I fully agree avoiding engine specific optimizations in frameworks would help make benchmarking efforts more deterministic. 

At the same time, I'm unsure what the best path forward is here given we know some frameworks actively do this (e.g Ember https://github.com/emberjs/ember.js/pull/15312/files do as well) in the name of perf. We can at best discourage them from doing so.

Would the preference for 175763 be that we stick to the existing version of Preact vs. upgrading to the latest?

[Ryosuke Niwa]

Created attachment 318920 [details]
Results

[Ryosuke Niwa]

It looks like the slowness in Safari comes from some other change than the one that made it faster in V8. From what I can tell, running Preact test case separately doesn't slow it down but only when I run it in succession. It's probably triggering a slightly different GC or current JIT behavior.

[Ryosuke Niwa]

Created attachment 318940 [details]
Experiment to revert 3f38738eb300a550a51088e3f6992c92685e89a5

Just to make sure 3f38738eb300a550a51088e3f6992c92685e89a5 didn't simultaneously speed up Chrome and slow down Safari, I've manually reverted the change and got 18 samples. The results indicate that the change neutral on Safari.

Given this, ~8% regression in Preact 7.1.0 to 8.1.0 on Safari is probably caused by some other changes in Preact.

[Ryosuke Niwa]

Comment on attachment 318857 [details]
Patch

Given these observations, it's probably to okay to merge this change.

[WebKit Commit Bot]

Comment on attachment 318857 [details]
Patch

Clearing flags on attachment: 318857

Committed r221121: <http://trac.webkit.org/changeset/221121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34048176>