Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update Glimmer VM #16268

Merged
merged 4 commits into from
Mar 5, 2018
Merged

Update Glimmer VM #16268

merged 4 commits into from
Mar 5, 2018

Conversation

chadhietala
Copy link
Contributor

@chadhietala chadhietala commented Feb 22, 2018 •
edited
Loading

This is the preemptive PR to integrate breaking changes from. glimmerjs/glimmer-vm#784

krisselden
krisselden reviewed Feb 27, 2018
View reviewed changes
.vscode/settings.json
@@ -0,0 +1,3 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this not the default?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that file is generated automatically by VS Code and should have not been committed & merged.

@chadhietala chadhietala changed the title [WIP] Update Glimmer Update Glimmer VM Mar 1, 2018
@chadhietala chadhietala force-pushed the update-glimmer branch 2 times, most recently from e2a6dd5 to 43f28a1 Compare March 2, 2018 15:41
@chadhietala
Copy link
Contributor Author

Wanted to run a benchmark on this sense to get the tests passing I needed to bump the Testem timeout.

TL;DR: Not stat-sig

Results
screen shot 2018-03-05 at 3 58 02 pm


                          ██
                          ██
         ██               ██
       ████               ████
       ████               ████
       ████  ██           ██████
       ████  ██           ██████
       ████████           ██████
       ████████         ████████
     ██████████         ████████
     ██████████         ████████
     ██████████       ████████████
   ████████████       ████████████
     duration              js
pr-16268
40 samples
Chrome/64.0.3282.186
8 x Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz

          ms
type              P10         Q1     Median         Q3        P90        MAD
  duration 2675.21120 2689.28300 2703.20000 2730.94800 2744.06920   27.30801
  js       2125.62020 2142.67075 2160.26800 2177.92950 2189.78760   26.19977
          ms
type              IQR
  duration   41.66500
  js         35.25875

     ██
     ██
     ██                         ██
     ██                       ████
     ██                       ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ████
     ████                     ██████
   ████████          ██       ██████        ██
         duration                    js
master-8ef8dc63cb
40 samples
Chrome/64.0.3282.186
8 x Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz

          ms
type             P10        Q1    Median        Q3       P90       MAD
  duration 2668.7274 2687.0222 2697.3945 2719.6492 2729.1470   25.1827
  js       2119.5275 2136.4772 2152.7870 2169.6402 2184.6161   25.5667
          ms
type             IQR
  duration   32.6270
  js         33.1630

Test pr-16268 against master-8ef8dc63cb

	Wilcoxon rank sum test

data:  ms by set
W = 906, p-value = 0.3118
alternative hypothesis: true location shift is not equal to 0
95 percent confidence interval:
 -5.734 18.373
sample estimates:
difference in location
                6.0235

rwjblue
rwjblue approved these changes Mar 5, 2018
View reviewed changes
packages/ember-glimmer/lib/setup-registry.ts
@@ -82,9 +82,9 @@ export function setupEngineRegistry(registry: Registry) {

registry.register('service:-glimmer-environment', Environment);

registry.register(P`template-options:main`, TemplateOptions);
registry.register(P`template-compiler:main`, TemplateCompiler);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iezer / @mixonic - ping (RE: updating the required values in ember-resolver when we add new types here)...

@rwjblue rwjblue merged commit 3240b4a into master Mar 5, 2018
@rwjblue rwjblue deleted the update-glimmer branch March 5, 2018 22:29
@chadhietala
Copy link
Contributor Author

chadhietala commented Mar 6, 2018 •
edited
Loading

@krisselden I ran the bench again with the Object.assign fixes. Looks like the IQR numbers improved

Still no stat-sig.

screen shot 2018-03-06 at 9 16 04 am

screen shot 2018-03-06 at 9 16 14 am

screen shot 2018-03-06 at 9 16 21 am


           ██
           ██                         ██
           ████                     ████
           ████                     ████
           ████                     ████
           ████                     ████
           ██████                   ████
           ██████                 ██████
         ████████                 ██████  ██
         ████████                 ██████████
     ██  ████████████         ██████████████
   ██████████████████       ████████████████████
        duration                     js
pr-16268
40 samples
Chrome/64.0.3282.186
8 x Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz

          ms
type              P10         Q1     Median         Q3        P90        MAD
  duration 2650.03100 2662.00775 2669.85900 2680.02625 2689.68290   12.61174
  js       2096.60710 2109.86250 2120.01250 2129.50000 2148.98710   14.40717
          ms
type              IQR
  duration   18.01850
  js         19.63750

       ██
       ██
       ████
       ████
       ████
       ████
       ██████
       ██████                     ██████
       ████████                   ██████  ██
       ████████             ██    ██████  ██
       ████████             ██████████████████
     ██████████             ██████████████████
   ██████████████       ██  ██████████████████
      duration                    js
master-8ef8dc63cb
40 samples
Chrome/64.0.3282.186
8 x Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz

          ms
type              P10         Q1     Median         Q3        P90        MAD
  duration 2641.74000 2651.99700 2672.37300 2692.28075 2705.51570   30.40664
  js       2089.50960 2107.60825 2127.55800 2140.59375 2153.34530   25.79057
          ms
type              IQR
  duration   40.28375
  js         32.98550

Test pr-16268 against master-8ef8dc63cb

	Wilcoxon rank sum test

data:  ms by set
W = 697, p-value = 0.3258
alternative hypothesis: true location shift is not equal to 0
95 percent confidence interval:
 -14.936   5.193
sample estimates:
difference in location
               -5.0895

@krisselden
Copy link
Contributor

if you have it handy, I would run with the runtimeStats

@cibernox cibernox mentioned this pull request Mar 9, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@btecu btecu btecu left review comments

@krisselden krisselden krisselden left review comments

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chadhietala @krisselden @rwjblue @btecu