Enable strings in the nursery
Categories
(Core :: JavaScript: GC, defect, P1)
Tracking
()
People
(Reporter: sfink, Assigned: sfink)
References
(Depends on 1 open bug, Blocks 6 open bugs)
Details
Attachments
(8 files, 4 obsolete files)
937 bytes,
patch
|
Details | Diff | Splinter Review | |
6.42 KB,
patch
|
jonco
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
961 bytes,
patch
|
jonco
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
10.00 KB,
patch
|
jonco
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
5.20 KB,
patch
|
jonco
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
12.48 KB,
patch
|
sfink
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
6.45 KB,
patch
|
jandem
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
7.20 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Bug 903519 implemented the functionality to allocate strings in the nursery, but did not enable it due to lingering bugs. This bug is for enabling it, probably in a different release than bug 903519 will land in. Note that nursery strings can already be enabled/disable with the JS shell command line option --nursery-strings=on/off, or the environment variable MOZ_NURSERY_STRINGS=0/1.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Just for the record, my current results for Octane are: median of 5 runs, inbound (nursery string disabled) -> devel (nursery strings enabled) (nursery) total: 1151963 -> 1174294 +22331 ( +1.9%) ( +1.94%) regression (nursery) mcWCll: 169571 -> 203312 +33741 (+19.9%) ( +2.93%) regression (nursery) mkRntm: 42652 -> 44949 +2297 ( +5.4%) ( +0.20%) regression (nursery) collct: 887012 -> 880616 -6396 ( -0.7%) ( -0.56%) improvement Richards: 26879 -> 26155 -724 ( -2.7%) ( -0.06%) regression DeltaBlue: 51579 -> 57840 +6261 (+12.1%) ( +0.54%) improvement Crypto: 26536 -> 27305 +769 ( +2.9%) ( +0.07%) improvement RayTrace: 114920 -> 107372 -7548 ( -6.6%) ( -0.66%) regression EarleyBoyer: 29252 -> 31686 +2434 ( +8.3%) ( +0.21%) improvement RegExp: 4488 -> 5460 +972 (+21.7%) ( +0.08%) improvement Splay: 17578 -> 19395 +1817 (+10.3%) ( +0.16%) improvement SplayLatency: 22096 -> 13819 -8277 (-37.5%) ( -0.72%) regression NavierStokes: 28408 -> 28556 +148 ( +0.5%) ( +0.01%) improvement PdfJS: 18062 -> 18842 +780 ( +4.3%) ( +0.07%) improvement Mandreel: 21686 -> 22464 +778 ( +3.6%) ( +0.07%) improvement MandreelLatency: 27696 -> 27850 +154 ( +0.6%) ( +0.01%) improvement Gameboy: 69999 -> 73461 +3462 ( +4.9%) ( +0.30%) improvement CodeLoad: 21721 -> 20686 -1035 ( -4.8%) ( -0.09%) regression Box2D: 48159 -> 48459 +300 ( +0.6%) ( +0.03%) improvement zlib: 78327 -> 81719 +3392 ( +4.3%) ( +0.29%) improvement Typescript: 26968 -> 27846 +878 ( +3.3%) ( +0.08%) improvement Score: 29414 -> 29921 +507 ( +1.7%) ( +0.04%) improvement The first percentage is that subtest's score change; the second is the subtest's effect on the final score. Note that nursery strings are disabled in this run early on in Splay, so the rest of the run has them disabled. There's quite a bit of variance. RegExp as usual is very happy, but it's a small test so doesn't contribute much to the result. SplayLatency is not happy about that; with some tuning of slice budgets or something, that'd probably be easy to improve, but the test's real-world value is low. Notice that "(nursery) total", the time spent collecting the nursery, regresses by 2%, but the final Octane score "Score" is pretty much unchanged. I'm thinking that the total effects on running time are minor GC time + faster allocation + more barriers. So I guess the faster allocation is compensating for more minor GC time? Either that, or Octane scoring is divorced from elapsed time, which is aggravated by the inclusion of the *Latency tests. Anyway, just a data point. Octane perf isn't holding this back; the prerequisite correctness bugs are.
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
Currently, JSString::flattenInternal uses the cell edge buffer for postbarriering ropes and dependent strings, which is overkill. And buggy, because we're transmuting the strings during the flattening, and apparently it's possible to end up tracing a string with data that doesn't match its specific string subtype. But this also simplifies the post-barriering substantially. Note that this does not solve all of the current problems. There appears to still be some issues with the pre-barriering. I haven't tracked that down yet, but again it seems like a pre-barrier can end up marking a string with mismatched flags and data. And I think this may even be happening when nursery strings are disabled?
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 3•6 years ago
|
||
This is the final patch to land, that will flip nursery strings on.
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
If we're going to generate a bunch of representative strings, we ought to have both tenured and nursery versions of them. Note that this has not uncovered any bugs, sadly.
Comment 6•6 years ago
|
||
Comment on attachment 8965916 [details] [diff] [review] Use whole cell buffer to post-barrier dependent strings Review of attachment 8965916 [details] [diff] [review]: ----------------------------------------------------------------- Took a while to work through this, but this seems fine.
Assignee | ||
Comment 7•6 years ago
|
||
This patch doesn't matter unless nursery strings are enabled, but I'd like to clear out some pending patches and this makes things simpler. And more correct -- the previous version was post-barriering some pointers to cells that could get mutated into being pointers to non-cells.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 8•6 years ago
|
||
Simple fix for an invariant that is no longer true once strings can be in the nursery.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 9•6 years ago
|
||
Somewhat questionable test addition. Perhaps it would be better to at least skip this if nursery strings are disabled in the Zone.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 10•6 years ago
|
||
Comment on attachment 8972664 [details] [diff] [review] Make nursery and tenured versions of representative test strings Review of attachment 8972664 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/StringType.cpp @@ +1810,5 @@ > + // that this is best-effort; if nursery strings are disabled, or we GC > + // midway through here, then we may end up with fewer nursery strings than > + // desired. > + mozilla::Maybe<gc::AutoSuppressNurseryCellAlloc> suppress; > + for (int loop = 0; loop < 2; loop++) { Might be simpler to factor the contents of the loop out into a separate function and call it twice.
Assignee | ||
Comment 11•6 years ago
|
||
While working on some testing infrastructure for this stuff, I scratched a long-time itch and moved a couple of test functions into a vm.string namespace: dumpStringRepresentation() -> vm.string.dump() representativeStringArray() -> vm.string.representativeArray() newExternalString() -> vm.string.newExternal() newMaybeExternalString() -> vm.string.newMaybeExternal() ensureFlatString -> vm.string.ensureFlat() and added a new vm.string.newRope(). I updated the test files accordingly, but I wanted to check with the fuzzers if this would be a major nuisance. If so, I could either (1) not do it, or (2) make the old names be aliases of the new ones so that nothing need change. (That's what I did in the past with things like os.file.readFile() == snarf().) On a related note, I did getBuildConfiguration() -> vm.buildConfig (a plain object, instead of function that returns a plain object.)
Assignee | ||
Comment 12•6 years ago
|
||
Oh, I wanted to mention two other possible intermediate solutions: (3) only make the aliases if fuzzing-safe, and (4) check in a file that contains a simple file like dumpStringRepresentation = vm.string.dump; ensureFlatString = vm.string.ensureFlat; ...etc... that fuzzing could include to provide backwards compatibility. (What I'm worried about is breaking bisection due to the renaming.)
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #12) > (What I'm worried about is breaking bisection due to the renaming.) You're right, it will. Only making the aliases if fuzzing-safe sounds like an acceptable compromise, but this might break edge cases where I had to bisect without --fuzzing-safe (bug 1459568) though it doesn't happen very often.
Assignee | ||
Comment 14•6 years ago
|
||
This is an addendum to the earlier patch to switch to the whole cell buffer for pointers coming from within strings. It also includes a test file that crashed without these changes, though I'll have to do some patch stack surgery to come up with an appliable sequencing.
Comment 15•6 years ago
|
||
Comment on attachment 8973926 [details] [diff] [review] Remove remaining string cell edge post barriers Review of attachment 8973926 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I like the test that constructs random ropes to exercise all the different possibilities. ::: js/src/tests/non262/String/ropes.js @@ +1,1 @@ > +print("Stress test of ropes"); Does this file need all the |var BUGNUMBER = ... | header that the other tests have?
Comment 16•6 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/102f4bab99db Use whole cell buffer to post-barrier string -> string edges, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/0187d0b8b762 Remove invariant that no longer is: ropes are not always tenured anymore, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/8f62649141c1 Make nursery and tenured versions of representative test strings, r=jonco
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/102f4bab99db https://hg.mozilla.org/mozilla-central/rev/0187d0b8b762 https://hg.mozilla.org/mozilla-central/rev/8f62649141c1
Assignee | ||
Comment 18•6 years ago
|
||
This should be [leave-open], that was just more prep work.
Updated•6 years ago
|
Assignee | ||
Comment 19•6 years ago
|
||
Moving over from the other bug, inheriting r+.
Assignee | ||
Comment 20•6 years ago
|
||
Don't emit unused post write barrier code when nursery strings are disabled.
Assignee | ||
Updated•6 years ago
|
Comment 22•6 years ago
|
||
Comment on attachment 8975927 [details] [diff] [review] make string post write barrier conditional on nursery strings Review of attachment 8975927 [details] [diff] [review]: ----------------------------------------------------------------- Nice.
Comment 23•6 years ago
|
||
Comment on attachment 8975927 [details] [diff] [review] make string post write barrier conditional on nursery strings Review of attachment 8975927 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +1597,5 @@ > + if (temp1.volatile_()) > + volatileRegs.add(temp1); > + > + masm.loadPtr(pendingInputAddress, temp2); > + masm.storePtr(input, pendingInputAddress); Actually you still need the store here and the one below I think?
Assignee | ||
Comment 24•6 years ago
|
||
Oops, neglected to put this back up for review. That [skipping those writes] was really dumb. Thanks. https://treeherder.mozilla.org/#/jobs?repo=try&revision=43a96cd1897ae0e0a54607bd059e1462361ff9b4 seems pretty happy.
Assignee | ||
Updated•6 years ago
|
Comment 25•6 years ago
|
||
Comment on attachment 8976617 [details] [diff] [review] make string post write barrier conditional on nursery strings Review of attachment 8976617 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Comment 26•6 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c67debf6be18 Use whole cell buffer to post-barrier string -> string edges in JIT, r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/3d5b6c62a386 make string post write barrier conditional on nursery strings
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c67debf6be18 https://hg.mozilla.org/mozilla-central/rev/3d5b6c62a386
Updated•6 years ago
|
Comment 28•6 years ago
|
||
There's no way we're taking this in 61.
Assignee | ||
Comment 30•6 years ago
|
||
Note that I don't have any more known problems here, I'm just waiting for the code freeze to thaw.
Comment 31•6 years ago
|
||
Half of the cycle is gone... Beta 9 is out.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 33•6 years ago
|
||
Comment on attachment 8972660 [details] [diff] [review] Use whole cell buffer to post-barrier dependent strings Review of attachment 8972660 [details] [diff] [review]: ----------------------------------------------------------------- (landed with https://bug1442481.bmoattachments.org/attachment.cgi?id=8973926)
Updated•6 years ago
|
Comment 34•6 years ago
|
||
Steve, should you ask Jon/Paul to review the last patch and get it landed?
Assignee | ||
Comment 35•6 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #34) > Steve, should you ask Jon/Paul to review the last patch and get it landed? No. There's still a test failure that I haven't managed to track down or reproduce.
Updated•6 years ago
|
Updated•5 years ago
|
I manage to reproduce the crash, the stack is https://pastebin.com/wwwSAUqu
in frame 4, or https://searchfox.org/mozilla-central/source/js/src/gc/Marking.cpp#1774
v.toString() is 0xfffe2f2f2f2f2f2f, I guess it's JS_FRESH_NURSERY_PATTERN, still checking this.
Assignee | ||
Comment 37•5 years ago
|
||
I finally managed to get another rr recording of the crash too. In my case, it's a missing post-barrier in JIT code. It was omitted because the Zone's allocNurseryStrings is false. It was turned off because there were too many tenured strings in that zone.
So... the only way I can see this making sense is if we're allocating a string in the nursery, then disabling nursery strings, then passing that nursery string into code that is compiled without nursery handling. Which makes no sense, since nursery strings should only be disabled at the end of a minor GC, and at the end of a minor GC there won't be any strings left in the nursery. (I'm trying to reverse-continue to when the nursery strings were disabled for that zone, but it's taking a very long time. And unfortunately this recording is large enough that it takes an hour to run to the point where it crashes.)
Perhaps this means that the incoming nursery string was not updated to its tenured location during that (or an earlier) minor GC? I'll see if I can find where that nursery string came from, though it's happening in JITted code so my success depends on how well rr behaves with watchpoints and things.
Assignee | ||
Comment 38•5 years ago
|
||
Tracked it down. The problem was that the JitRealm was not calculating whether nursery strings were enabled properly. It was set to true if nursery strings were enabled, ignoring the flag that turned them off on the Zone. Though I'm not sure if that came in when rebasing on top of realms? I should look at the version before that.
It seemed like Yoshi's case, though, might not be in JIT code, in which case it would be something else?
Assignee | ||
Comment 39•5 years ago
|
||
Stupid implicit constructor mistake in that push. Much better:
The SM(r) red is bogus. It's from timeouts that are due to either some expensive checks that I have in my patch stack, or from some MOZ_NEVER_INLINEs I injected for better debugging; I'm not sure which. It doesn't really matter.
It seemed like Yoshi's case, though, might not be in JIT code, in which case it would be something else?
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=221914620&revision=2e0257b04bafb969ec9cbdcfc8d235bac905418e
I tried your patch and the try looks green now (with theexplicit
on the JitRealm cstor to fix the build error)
Assignee | ||
Comment 41•5 years ago
|
||
JitRealm's stringsCanBeInNursery must be based on current state of Zone, not the global setting for the whole runtime.
Comment 42•5 years ago
|
||
Comment on attachment 9039298 [details] [diff] [review] Fix JitRealm nursery strings test Review of attachment 9039298 [details] [diff] [review]: ----------------------------------------------------------------- Nice find!
Comment 43•5 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc51ff3d0035 Fix JitRealm nursery strings test, r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/b7be8fab1b5e Allocate strings in the nursery, r=flagflip
Comment 44•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•5 years ago
|
Description
•