Bug 175763

Created attachment 318618 [details] Patch

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.

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

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

(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.

(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? :)

(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 on attachment 318618 [details] Patch With this version, when marking todo items as completed, the items are no longer grayed out with strikethrough.

Created attachment 318857 [details] Patch

(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.

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.

> 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?

Created attachment 318920 [details] Results

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.

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 on attachment 318857 [details] Patch Given these observations, it's probably to okay to merge this change.

Comment on attachment 318857 [details] Patch Clearing flags on attachment: 318857 Committed r221121: <http://trac.webkit.org/changeset/221121>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34048176>