Closed Bug 1442481 Opened 2 years ago Closed 10 months ago

Enable strings in the nursery

Categories

(Core :: JavaScript: GC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Depends on 2 open bugs, Blocks 9 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.
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.
Priority: -- → P1
Depends on: 1444335
Depends on: 1446693
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: nobody → sphink
Status: NEW → ASSIGNED
This is the final patch to land, that will flip nursery strings on.
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 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.
Attachment #8965916 - Flags: feedback+
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.
Attachment #8972660 - Flags: review?(jcoppeard)
Attachment #8965916 - Attachment is obsolete: true
Simple fix for an invariant that is no longer true once strings can be in the nursery.
Attachment #8972663 - Flags: review?(jcoppeard)
Attachment #8965918 - Attachment is obsolete: true
Somewhat questionable test addition. Perhaps it would be better to at least skip this if nursery strings are disabled in the Zone.
Attachment #8972664 - Flags: review?(jcoppeard)
Attachment #8965919 - Attachment is obsolete: true
Attachment #8972660 - Flags: review?(jcoppeard) → review+
Attachment #8972663 - Flags: review?(jcoppeard) → review+
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.
Attachment #8972664 - Flags: review?(jcoppeard) → review+
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.)
Flags: needinfo?(nth10sd)
Flags: needinfo?(choller)
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.
Flags: needinfo?(nth10sd)
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.
Attachment #8973926 - Flags: review?(jcoppeard)
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?
Attachment #8973926 - Flags: review?(jcoppeard) → review+
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
This should be [leave-open], that was just more prep work.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Target Milestone: mozilla62 → ---
Flags: needinfo?(choller)
Moving over from the other bug, inheriting r+.
Don't emit unused post write barrier code when nursery strings are disabled.
Attachment #8975927 - Flags: review?(jdemooij)
Attachment #8975926 - Flags: review+
Duplicate of this bug: 1459625
Comment on attachment 8975927 [details] [diff] [review]
make string post write barrier conditional on nursery strings

Review of attachment 8975927 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8975927 - Flags: review?(jdemooij) → review+
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?
Attachment #8975927 - Flags: review+
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.
Attachment #8976617 - Flags: review?(jdemooij)
Attachment #8975927 - Attachment is obsolete: true
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.
Attachment #8976617 - Flags: review?(jdemooij) → review+
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
Depends on: 1463639
There's no way we're taking this in 61.
Depends on: 1482993
Depends on: 1482178
Depends on: 1470921
Duplicate of this bug: 1443607
Note that I don't have any more known problems here, I'm just waiting for the code freeze to thaw.
Half of the cycle is gone... Beta 9 is out.
Going to freeze again Monday!
Flags: needinfo?(sphink)
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)
Attachment #8972660 - Flags: checkin+
Steve, should you ask Jon/Paul to review the last patch and get it landed?
Flags: needinfo?(sphink)
(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.
Blocks: 1507445

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.

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.

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?

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=221914620&revision=2e0257b04bafb969ec9cbdcfc8d235bac905418e

Stupid implicit constructor mistake in that push. Much better:

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=221914620&revision=9e3b4a4d3bd9525a2e16d70d12c0348dd9e2b8c6

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 the explicit on the JitRealm cstor to fix the build error)

Blocks: 1522186
Blocks: 1522187
Blocks: 1522189
JitRealm's stringsCanBeInNursery must be based on current state of Zone, not the global setting for the whole runtime.
Attachment #9039298 - Flags: review?(jdemooij)
Comment on attachment 9039298 [details] [diff] [review]
Fix JitRealm nursery strings test

Review of attachment 9039298 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find!
Attachment #9039298 - Flags: review?(jdemooij) → review+
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
Keywords: leave-open
Target Milestone: --- → mozilla67
Status: REOPENED → RESOLVED
Closed: 2 years ago10 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.