Disable and remove Unboxed Objects

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: mgaudet, Assigned: mgaudet)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

Attachments

(20 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
There are a number of downsides to unboxed object representation which may justify its disablement and removal.

However, in some workloads it does provide a performance benefit, so needs to be considered.
A try push where I flip the sense of the unboxed objects flag, disabling it by default: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=a9f421fc69b3&framework=10&showOnlyComparable=1&selectedTimeRange=172800

- Early results are mixed for speedometer: Some subtests show substantial losses, others gains.
    - The "Inferno TodoMVC" tests seem particulraly bad, with some tests seeing 20% slowdowns. 
    - The "Vanilla-ES2015-Babel-Webpack" tests seem particularly improved, with some tests showing up to 10% improvement. 

I've also kicked off a second push, with Unboxed objects disabled, but the object construction code from Bug 1488579 active: We'll see what the results are there tomorrow [1] 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c0242729b0596fc1c7f21f49ce1530f5db4319e
Results Links (be sure to use the dropdown box to select Raptor to see speedometer results)

1. Disable Unboxed Objects, Compared to Mozilla Central, 2 day window: https://mzl.la/2z1Vppg
2. Disable Unboxed Objects + Fast Plain Object Construction (Bug 1488579), Compared to Mozilla Central, 2 day window: https://mzl.la/2zBRvmy
3. (1) compared with (2), to see how much Bug 1488579 helps: https://mzl.la/2Plr2oh

Some Notes on looking at the results. Most of my analysis will focus on the linux64 numbers (PGO in the case of speedmoeter to best match a real-world deployment); eyeballing it, I don't seem much in the way of significant difference between platforms (as you would expect!).
 

On the Unboxed Objects Removal (1): 

- For the most part, the top-line scores are not particularly affected, except for Kraken, which was expected. 
  - As I understand it the difference between linux64 and linux64-qr is that the latter has webrender force-enabled, yet, the kraken results are markedly different between the runs. Not sure how much value to put in this... I don't really trust Kraken as a suite. 
  - In some modes, we also see a substantial loss to ai-astar. 

- Subtest scores show offsetting impacts: A number of wins, matched by an equal weighting of losses. 
  - Despite that weighting, there are fewer (13) 'significant' wins than significant losses (19 + 3 medium confidence), and there are larger losses;  Inferno-TodoMVC/CompletingAllItems pgo loses 23%, 


On the Fast Plain Objects fix (2,3): 

- This improves 10% on kraken over the baseline of removing unboxed objects. 
- Doesn't restore any of the speedometer losses 
- Causes a couple increased slowdowns on speedometer (3% on one subtest) 


Closing thoughts: 

- It seems like it would be worthwhile to open a bug to investigate the large unboxed object performance losses on the speedometer subtests to see if we can devise a strategy to restore performance there. 
- I'm curious about how much of the impact (or lacktherof) we see from the Fast Plain Objects patch is due to tuning: I'm going to try a lower threshold run for my own edification. 

What do we think about flipping the switch?
Assignee

Updated

8 months ago
Blocks: shypes
Depends on: 1488579
Assignee

Updated

8 months ago
Depends on: 1505807
(Tried getting results for Octane, ares and webtooling, but it seems to have gone totally sideways, Bug 1505853)
:denispal was kind enough to do a weekend run on some real-world websites. The patch tested was https://hg.mozilla.org/try/rev/5b6ce394a99a. The first column is the base (he used 5e7636ec12c5), the second column is base+patch. 

url                                avg ( std, count)         avg ( std, count)   speedup
------------------------------------------------------------------------------------------
http://newsnow.co.uk           2859.44 (  2%,    32)     2846.70 (  1%,    30)      0.45%
http://time.com                4842.94 (  5%,    35)     4440.03 (  6%,    33)      8.32%
http://indiatimes.com          3466.30 (  7%,    81)     3457.42 (  4%,    65)      0.26%
http://nytimes.com             4129.10 (  6%,    40)     3973.22 (  6%,    40)      3.78%
http://huffingtonpost.com      4360.18 (  5%,    33)     4311.89 (  9%,    36)      1.11%
http://chicagotribune.com      5215.08 (  9%,    39)     5118.97 (  5%,    36)      1.84%
http://cnbc.com                3915.39 (  8%,    33)     3551.87 (  7%,    30)      9.28%
http://theatlantic.com         1138.60 (  7%,    35)     1122.67 (  8%,    36)      1.40%
http://thehindu.com            4410.03 (  9%,    33)     3999.70 (  8%,    40)      9.30%
http://dw.com                  3761.91 ( 10%,    34)     2806.07 (  8%,    28)     25.41%
http://variety.com             8500.28 (  8%,    29)     8583.53 ( 10%,    32)     -0.98%
http://usatoday.com            5438.15 (  9%,    39)     5452.75 ( 10%,    36)     -0.27%
http://shutterstock.com        1733.22 (  7%,    32)     1678.03 ( 12%,    34)      3.18%
http://washingtonpost.com      4389.72 (  9%,    32)     4147.67 ( 12%,    30)      5.51%
http://thedailybeast.com       3536.53 ( 12%,    32)     3417.94 (  9%,    33)      3.35%
http://reddit.com              3760.83 ( 14%,    36)     3669.95 ( 10%,    37)      2.42%
http://weather.com             1479.58 ( 19%,    38)     1258.66 (  7%,    35)     14.93%
http://wsj.com                 6181.27 ( 16%,    37)     5826.73 ( 11%,    37)      5.74%
http://hollywoodreporter.com   4361.38 ( 18%,    32)     4133.69 ( 13%,    35)      5.22%
http://latimes.com             4508.23 ( 18%,    31)     4426.07 ( 16%,    29)      1.82%
http://theguardian.com         2270.28 ( 27%,    36)     1989.90 ( 11%,    40)     12.35%
http://sfgate.com              6096.56 ( 14%,    34)     6150.79 ( 42%,    38)     -0.89%
http://news.com.au             4514.85 ( 29%,    33)     5004.66 ( 29%,    32)    -10.85%
http://weather.gov             1423.95 ( 38%,    37)     1109.54 ( 23%,    37)     22.08%
http://thehill.com             3641.54 ( 21%,    28)     3227.82 ( 48%,    38)     11.36%
http://foxnews.com             4861.38 ( 40%,    32)     4638.91 ( 38%,    34)      4.58%
http://cnn.com                 6680.13 ( 36%,    31)     5323.49 ( 49%,    39)     20.31%
http://forbes.com              4541.19 ( 23%,    37)     4657.56 ( 80%,    39)     -2.56%
http://chron.com               7368.43 ( 64%,    35)     6767.15 ( 50%,    33)      8.16%
http://abcnews.go.com          5741.79 ( 50%,    39)     6708.47 ( 86%,    34)    -16.84%
I ran this again,  but this time using a new framework that has statically recorded pages to reduce the noise considerably.  Still seems like a win here:

url                                                     avg ( std, count)         avg ( std, count)     speedup
---------------------------------------------------------------------------------------------------------------

https://www.abcnews.com                             1375.04 (  1%,    28)     1378.14 (  1%,    29)     -0.23%
https://www.wunderground.com/                       4881.31 (  3%,    26)     4815.24 (  4%,    29)      1.35%
https://www.reddit.com                              4218.71 (  4%,    28)     4175.47 (  4%,    34)      1.03%
https://www.huffingtonpost.com                      4245.31 (  4%,    29)     4176.51 (  2%,    35)      1.62%
https://www.expedia.com                             4560.33 (  4%,    24)     4403.00 (  3%,    30)      3.45%
https://www.sfgate.com                              7613.48 (  4%,    27)     6974.04 (  7%,    28)      8.40%
https://www.facebook.com/DwayneJohnson/             2873.31 (  5%,    26)     2865.40 (  6%,    30)      0.28%
https://www.thehindu.com                            4689.21 (  5%,    24)     4450.82 (  4%,    28)      5.08%
https://www.dw.com                                  3147.73 (  5%,    26)     3220.11 (  7%,    37)     -2.30%
https://www.usnews.com                              3135.63 (  5%,    27)     2988.77 (  6%,    30)      4.68%
https://www.news.com.au                             4549.22 (  5%,    27)     4504.87 (  8%,    31)      0.97%
https://www.nytimes.com                             3258.70 (  5%,    27)     3221.42 (  6%,    33)      1.14%
https://www.hindustantimes.com                      2505.41 (  5%,    29)     2492.27 (  4%,    30)      0.52%
https://www.hollywoodreporter.com                   4901.57 (  5%,    30)     4782.25 (  4%,    36)      2.43%
https://www.theguardian.com/international           3594.57 (  6%,    30)     3422.55 (  4%,    29)      4.79%
https://www.cnbc.com                                3980.62 (  7%,    26)     3877.62 (  8%,    26)      2.59%
https://www.thedailybeast.com                       3302.77 (  7%,    26)     3163.82 (  5%,    28)      4.21%
https://www.cbc.ca                                  3188.82 (  8%,    28)     3019.00 (  6%,    30)      5.33%
https://www.foxnews.com                             5171.42 (  8%,    26)     5224.78 (  7%,    36)     -1.03%
https://www.cbsnews.com                             3018.85 (  9%,    26)     3266.41 (  9%,    27)     -8.20%
https://www.cnn.com                                 5190.07 (  9%,    27)     4652.18 (  6%,    28)     10.36%
https://www.latimes.com                             3062.59 ( 10%,    27)     2878.61 ( 12%,    31)      6.01%
https://www.instagram.com/justintimberlake/         1233.58 ( 10%,    33)     1098.00 (  4%,    31)     10.99%
https://www.indiatimes.com                          3306.47 ( 10%,    30)     3137.94 (  6%,    31)      5.10%
https://docs.google.com/document/d/1CBF65WS71QOC4XiUH-KT2ZGDTp5ieSMSqC-d1NfoFCw/edit#heading=h.ptbvkvnv25r7 12260.17 (  1%,    30)    11912.94 (  7%,    33)      2.83%


Geomean is about +3%.
Assignee

Updated

5 months ago
Depends on: 1525579
Assignee

Updated

5 months ago
Blocks: 1526451
Blocks: 1531842
Assignee

Updated

3 months ago
Blocks: 1536433
Assignee

Updated

3 months ago
Blocks: 1536439
Assignee

Updated

3 months ago
Blocks: 1536460

A note about the Scalar Replacement Changes: The M{Load,Store}Unboxed*
instructions in theory could be used to manipulate and analyze typed arrays.

However, according to nbp TypedArrays should already be excluded from elibility
because of the potential for cross-thread sharing in a SharedArrayBuffer world,
and so the only support in Scalar Replacement here is for Unboxed Objects, and
so it can all be removed.

Depends on D24037

A note about the patches: They're sort of awkwardly batched to allow partial bisection on the off-chance something went poorly, however, there aren't a lot of good cut-points, so this batching is extremely arbitrary.

Assignee

Updated

3 months ago
Blocks: 1536880

This is possible now that we no longer have unboxed objects

Depends on D24247

Depends on D24250

Assignee

Updated

3 months ago
Duplicate of this bug: 1536460

One thing to note from some preliminary Talos testing was a substantial reddit regression on TTFI

Local testing also shows a degradation.

> central_ttfi <- c(9973, 9714, 8756, 11702, 12911, 9143, 8863, 8909, 8416, 8798, 8813, 13037, 11030, 9315, 8646, 8775, 9004, 14002, 9653, 10850, 7302, 7738, 7810, 7621, 8293)
> test_ttfi <- c[(10650, 9943, 14728, 9788, 11818, 9153, 9102, 8703, 8393, 8825, 9023, 9006, 14849, 8661, 9558, 9559, 17954, 10607, 11025, 9390, 12824, 10473, 14234, 9320, 12865)
> t.test(test_ttfi,central_ttfi)

	Welch Two Sample t-test

data:  test_ttfi and central_ttfi
t = 2.0795, df = 43.587, p-value = 0.04349
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
   38.35968 2471.80032
sample estimates:
mean of x mean of y 
 10818.04   9562.96 

Looking at the historical graphs for that measure it does seem to be noisy, though my patch values do seem higher noticeably. I've kicked off another try run to get some more data.

I don't think we should block landing based on this, however I'll have to do some investigation to see if I can figure out -why- this is happening. Unboxed objects are already disabled, so this will have to be some sort of secondary effect.

Interesting. Yeah it's worth investigating - could be a bug in one of the patches.

So, I've been unable to locally bisect this as the noise on my machine seems to render local runs useless.

I've observed on the same revision huge swings. e.g

  • on Central, without my changes entirely, an average of both 8886 and 9844.
  • on tip, with all my changes, I see both 9673.5 and 9857.0

Looking at the raw measurements, you can see that they're all over the place.

central: {"name": "ttfi", "lowerIsBetter": true, "alertThreshold": 2.0, "value": 8886.0, "replicates": [9973, 9714, 8756, 11702, 12911, 9143, 8863, 8909, 8416, 8798, 8813, 13037, 11030, 9315, 8646, 8775, 9004, 14002, 9653, 10850, 7302, 7738, 7810, 7621, 8293], "unit": "ms”},
         {"name": "ttfi", "lowerIsBetter": true, "alertThreshold": 2.0, "value": 9844.0, "replicates": [10515, 9052, 9123, 15871, 9873, 9298, 8412, 13977, 8543, 8824, 8432, 8672, 10008, 9641, 9985, 9815, 11812, 10331, 10993, 10055, 16356, 11266, 10256, 9801, 9536], "unit": "ms"}

tip:     {"name": "ttfi", "lowerIsBetter": true, "alertThreshold": 2.0, "value": 9673.5, "replicates": [10650, 9943, 14728, 9788, 11818, 9153, 9102, 8703, 8393, 8825, 9023, 9006, 14849, 8661, 9558, 9559, 17954, 10607, 11025, 9390, 12824, 10473, 14234, 9320, 12865], "unit": "ms”},
         {"name": "ttfi", "lowerIsBetter": true, "alertThreshold": 2.0, "value": 9857.0, "replicates": [9395, 9496, 9283, 9594, 10512, 9743, 13349, 11136, 12809, 9426, 9233, 9462, 10665, 9247, 9734, 9576, 19750, 10306, 9684, 9808, 12217, 9906, 11283, 10462, 11598], "unit": "ms"}

I'm going to try and see if I see similar wild swings on try.

The reddit regression disappeared with a new Try build comparison

Must have been some unlucky noise.

Comment 32

3 months ago
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0330a759e399
Unhook Unboxed Objects option and disable by default r=iain
https://hg.mozilla.org/integration/autoland/rev/4e8e9a0b6c69
Remove initial chunk of Unboxed Objects machinery r=iain
https://hg.mozilla.org/integration/autoland/rev/ef947b198f2c
Remove Unboxed Objects in ScalarReplacement r=nbp
https://hg.mozilla.org/integration/autoland/rev/7e790d902c60
Remove Unboxed Objects from Builtins Directory r=iain
https://hg.mozilla.org/integration/autoland/rev/3abc5dfef71f
Remove Unboxed Objects support from GC r=jandem
https://hg.mozilla.org/integration/autoland/rev/551ced45a264
Remove Unboxed Objects support from Iteration.cpp r=iain
https://hg.mozilla.org/integration/autoland/rev/35a9aa1ae7b9
Remove Unboxed Objects from vm/ Part 1 r=iain
https://hg.mozilla.org/integration/autoland/rev/2939d41f1ade
Remove Unboxed Objects from jit/ - Part 1 r=iain
https://hg.mozilla.org/integration/autoland/rev/8d759131d76e
Remove Unboxed Objects from vm/ - Part 2 r=iain
https://hg.mozilla.org/integration/autoland/rev/c54c4aa147f9
Remove Unboxed Objects from jit/ - Part 2 r=iain
https://hg.mozilla.org/integration/autoland/rev/6b55560f4da9
Remove Unboxed Objects from jit/ - Part 3 r=iain
https://hg.mozilla.org/integration/autoland/rev/d4383df46286
Remove Unboxed Objects from vm/ - Part 3 r=iain
https://hg.mozilla.org/integration/autoland/rev/fa973fb70a32
Remove Unboxed Objects Option Code r=iain
https://hg.mozilla.org/integration/autoland/rev/140d92e854c1
Remove Unboxed Objects Test code r=iain
https://hg.mozilla.org/integration/autoland/rev/164b08106106
Remove UnboxedObjects ObjectGroup addendum r=iain
https://hg.mozilla.org/integration/autoland/rev/2f776fa2433d
Remove now extraneous template parameter r=iain
https://hg.mozilla.org/integration/autoland/rev/eb04dcf207a0
Prohibit class changes on groups r=iain
https://hg.mozilla.org/integration/autoland/rev/a19a0fc91ddc
Remove unused parameters from Unboxed Objects r=iain
https://hg.mozilla.org/integration/autoland/rev/136b21be1299
Clean out MObjectState r=iain
https://hg.mozilla.org/integration/autoland/rev/89945a066c66
Remove anyNewScript r=iain

Comment 33

3 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/6ba48873ecae
Re-delete UnboxedObjects.cpp added back by backout. r=mgaudet CLOSED TREE
Assignee: nobody → mgaudet

Seems like Assorted DOM returned to previous baselines.

== Change summary for alert #20081 (as of Sat, 23 Mar 2019 01:09:14 GMT) ==

Improvements:

5% raptor-assorted-dom-firefox linux64-pgo-qr opt 26.57 -> 25.25
5% raptor-assorted-dom-firefox linux64 pgo 26.39 -> 25.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20081

You need to log in before you can comment on or make changes to this bug.