Bug 175926 - Speedometer: Restructure Ember example
Summary: Speedometer: Restructure Ember example
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: 175956
Blocks: 172339
  Show dependency treegraph
 
Reported: 2017-08-24 00:04 PDT by Mathias Bynens
Modified: 2017-08-25 15:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch (32.36 KB, patch)
2017-08-24 00:05 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Patch (32.37 KB, patch)
2017-08-24 00:19 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Patch (3.95 MB, patch)
2017-08-24 01:51 PDT, Mathias Bynens
no flags Details | Formatted Diff | Diff
Results before and after applying the patch (93.82 KB, application/pdf)
2017-08-24 02:18 PDT, Mathias Bynens
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-24 00:04:12 PDT
Speedometer: Remove unused stylesheet in Ember example
Comment 1 Mathias Bynens 2017-08-24 00:05:54 PDT
Created attachment 318971 [details]
Patch
Comment 2 Mathias Bynens 2017-08-24 00:14:24 PDT
I know the diff looks massive (because of the generated + minified files), but don’t despair! Only the hashes changed.

E.g. the only difference in todomvc-*.js is the `version` hash at the bottom. Old version:

    version: "0.0.0+164d27e2"

New version:

    version: "0.0.0+a2ca1650"
Comment 3 Mathias Bynens 2017-08-24 00:19:47 PDT
Created attachment 318972 [details]
Patch
Comment 4 Ryosuke Niwa 2017-08-24 01:28:07 PDT
Comment on attachment 318972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318972&action=review

> PerformanceTests/ChangeLog:10
> +        * Speedometer/resources/todomvc/architecture-examples/emberjs/source/app/index.html: Removed stylesheet reference.
> +        * Speedometer/resources/todomvc/architecture-examples/emberjs/source/app/styles/.gitkeep: Added.
> +        * Speedometer/resources/todomvc/architecture-examples/emberjs/source/dist/*: Regenerated per build instructions.

Something is not right. tests.js refer to todomvc/architecture-examples/emberjs/index.html yet this is re-generating emberjs/source/dist/index.html:

Suites.push({
    name: 'EmberJS-TodoMVC',
    url: 'todomvc/architecture-examples/emberjs/index.html',
Comment 5 Mathias Bynens 2017-08-24 01:51:26 PDT
Created attachment 318976 [details]
Patch
Comment 6 Mathias Bynens 2017-08-24 01:54:39 PDT
(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 318972 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318972&action=review
> 
> > PerformanceTests/ChangeLog:10
> > +        * Speedometer/resources/todomvc/architecture-examples/emberjs/source/app/index.html: Removed stylesheet reference.
> > +        * Speedometer/resources/todomvc/architecture-examples/emberjs/source/app/styles/.gitkeep: Added.
> > +        * Speedometer/resources/todomvc/architecture-examples/emberjs/source/dist/*: Regenerated per build instructions.
> 
> Something is not right. tests.js refer to
> todomvc/architecture-examples/emberjs/index.html yet this is re-generating
> emberjs/source/dist/index.html:
> 
> Suites.push({
>     name: 'EmberJS-TodoMVC',
>     url: 'todomvc/architecture-examples/emberjs/index.html',

Good catch. That should’ve been `todomvc/architecture-examples/emberjs/source/dist/index.html` all along.

EmberJS is the only test with such a weird directory structure: `/emberjs` is pretty much empty, `/emberjs/source` contains the actual source, but the output is written to `/emberjs/source/dist`. The new patch fixes that as well by getting rid of the intermediate `source` folder.
Comment 7 Mathias Bynens 2017-08-24 02:18:58 PDT
Created attachment 318978 [details]
Results before and after applying the patch
Comment 8 Ryosuke Niwa 2017-08-24 13:46:15 PDT
Okay, this results in a dramatic performance improvements in both Safari & Chrome. The magnitude of the change here is almost 4x. I don't think we can accept this change as is.

== Before ==

Safari
34.6
34.9
34.7

Chrome
28.0
28.4
28.1

== After ==

Safari
108
107
109

Chrome
109
107
110
Comment 9 Ryosuke Niwa 2017-08-25 14:25:35 PDT
Comment on attachment 318976 [details]
Patch

Now that https://trac.webkit.org/changeset/221205 has been landed, we can land this.
Comment 10 WebKit Commit Bot 2017-08-25 15:00:56 PDT
Comment on attachment 318976 [details]
Patch

Clearing flags on attachment: 318976

Committed r221206: <http://trac.webkit.org/changeset/221206>
Comment 11 WebKit Commit Bot 2017-08-25 15:00:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-08-25 15:04:27 PDT
<rdar://problem/34089824>