Closed Bug 1505574 Opened 7 years ago Closed 7 years ago

Disable and remove Unboxed Objects

Categories

(Core :: JavaScript Engine, enhancement, P3)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: mgaudet, Assigned: mgaudet)

References

Details

Attachments

(20 files)

Bug 1505574 - Unhook Unboxed Objects option and disable by default r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove initial chunk of Unboxed Objects machinery r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects in ScalarReplacement r?nbp
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects from Builtins Directory r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects support from GC r?pbone
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects support from Iteration.cpp r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects from vm/ Part 1 r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects from jit/ - Part 1 r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects from vm/ - Part 2 r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects from jit/ - Part 2 r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects from jit/ - Part 3 r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects from vm/ - Part 3 r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects Option Code r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove Unboxed Objects Test code r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove UnboxedObjects ObjectGroup addendum r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove now extraneous template parameter r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Prohibit class changes on groups r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove unused parameters from Unboxed Objects r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Clean out MObjectState r?iain
47 bytes, text/x-phabricator-request
Details | Review
Bug 1505574 - Remove anyNewScript r?iain
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?
Blocks: shypes
Depends on: 1488579
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%.
Depends on: 1525579
Blocks: 1526451
Blocks: 1536433
Blocks: 1536439
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.

Blocks: 1536880

This is possible now that we no longer have unboxed objects

Depends on D24247

Depends on D24249

Depends on D24250

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.

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

Regressions: 1620193
Regressions: CVE-2020-12406
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: