Bug 175763 - Speedometer: Update to modern Preact version
Summary: Speedometer: Update to modern Preact version
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 172339
  Show dependency treegraph
 
Reported: 2017-08-21 01:04 PDT by Mathias Bynens
Modified: 2017-08-23 17:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (317.72 KB, patch)
2017-08-21 01:08 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Results before and after the patch (93.49 KB, application/pdf)
2017-08-21 05:49 PDT, Mathias Bynens
no flags Details
Patch (318.93 KB, patch)
2017-08-23 01:13 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Results (19.68 KB, application/pdf)
2017-08-23 14:33 PDT, Ryosuke Niwa
no flags Details
Experiment to revert 3f38738eb300a550a51088e3f6992c92685e89a5 (581.84 KB, application/x-iwork-keynote-sffnumbers)
2017-08-23 16:43 PDT, Ryosuke Niwa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mathias Bynens 2017-08-21 01:04:13 PDT
Speedometer: Update to modern Preact version
Comment 1 Mathias Bynens 2017-08-21 01:08:20 PDT
Created attachment 318618 [details]
Patch
Comment 2 Mathias Bynens 2017-08-21 01:15:57 PDT
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.
Comment 3 Mathias Bynens 2017-08-21 05:49:34 PDT
Created attachment 318629 [details]
Results before and after the patch
Comment 4 Ryosuke Niwa 2017-08-21 14:45:02 PDT
Why did Firefox's score got much worse in this change? 87 -> 78
Comment 5 Ryosuke Niwa 2017-08-21 23:02:26 PDT
(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.
Comment 6 Mathias Bynens 2017-08-22 01:20:23 PDT
(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? :)
Comment 7 Ryosuke Niwa 2017-08-22 13:55:23 PDT
(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.
Comment 8 Ryosuke Niwa 2017-08-22 16:20:14 PDT
Comment on attachment 318618 [details]
Patch

With this version, when marking todo items as completed, the items are no longer grayed out with strikethrough.
Comment 9 Mathias Bynens 2017-08-23 01:13:10 PDT
Created attachment 318857 [details]
Patch
Comment 10 Mathias Bynens 2017-08-23 01:13:32 PDT
(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.
Comment 11 Ryosuke Niwa 2017-08-23 14:07:48 PDT
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.
Comment 12 Addy Osmani 2017-08-23 14:27:50 PDT
> 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?
Comment 13 Ryosuke Niwa 2017-08-23 14:33:22 PDT
Created attachment 318920 [details]
Results
Comment 14 Ryosuke Niwa 2017-08-23 14:47:29 PDT
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.
Comment 15 Ryosuke Niwa 2017-08-23 16:43:24 PDT
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.
Comment 16 Ryosuke Niwa 2017-08-23 16:45:34 PDT
Comment on attachment 318857 [details]
Patch

Given these observations, it's probably to okay to merge this change.
Comment 17 WebKit Commit Bot 2017-08-23 17:15:01 PDT
Comment on attachment 318857 [details]
Patch

Clearing flags on attachment: 318857

Committed r221121: <http://trac.webkit.org/changeset/221121>
Comment 18 WebKit Commit Bot 2017-08-23 17:15:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2017-08-23 17:15:34 PDT
<rdar://problem/34048176>