1376646 - Optimize store buffer tracing for nursery-allocatable cells

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

I doubt it's a big enough perf problem to block the landing, so I'm splitting this out into a separate bug.

I added a dynamic check to StoreBuffer::CellPtrEdge::trace() to determine whether we're looking at a string or an object, and I *think* it can be done away with by making a separate store buffer for each. (Though I also think that it may be easier in the JIT to not distinguish during insertion?)

Also, I have a mostly redundant IsInsideNursery check that could be removed with some refactoring.

Comment 1

[Ted Campbell [:tcampbell]]

Do we still think we can do this? It would be really nice to remove nurseryCellIsString which does some really undesirable reinterpret_casts. Otherwise, I may consider adding a MaybeNurseryCell base-class to JSObject/JSString containing the first uintptr_t in one place to be more explicit about how these types have been woven together.

(Sorry I'm being a pain about this, but I'd really love to kill our bad habits about reinterpret_casts and make the subtle VM interactions more explicit. I'm optimistic that this is possible and worth doing.)

Comment 2

[Steve Fink [:sfink] [:s:]]

I still think we can do this, yes.

The other thing involved here is independent of nursery strings: when storing Cells in the nursery, we also lie to the compiler and overlay them with a forwarding marker and info: https://searchfox.org/mozilla-central/source/js/src/gc/RelocationOverlay.h#32

The interactions with that are even more subtle. Though I guess they got substantially worse with nursery strings. But either way, it's a reinterpret_cast, with a carefully-placed magic_ field that should never show up in a real Cell.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Sfink
So is the fix is to have one bufferCell for JSString and another bufferCell for JSOBject?
https://searchfox.org/mozilla-central/rev/f4c39907e0b527dc4b9356a1eeb8c6e6c62d383a/js/src/gc/StoreBuffer.h#420

And we will have another bool to tell CellPtrEdge the edge is for JSString or JSObject.

And for 'only check IsInsideNursery once.'
https://searchfox.org/mozilla-central/rev/f4c39907e0b527dc4b9356a1eeb8c6e6c62d383a/js/src/gc/Marking.cpp#2792

Do you mean that because we already check if the cell is in nursery in maybeInRemberedSet
https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/StoreBuffer.h#255

So we can remove the IsInsideNursery here?
https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/Marking.cpp#2794

Thanks

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #3)

Hi Sfink
So is the fix is to have one bufferCell for JSString and another bufferCell for JSOBject?
https://searchfox.org/mozilla-central/rev/f4c39907e0b527dc4b9356a1eeb8c6e6c62d383a/js/src/gc/StoreBuffer.h#420

Yes.

And we will have another bool to tell CellPtrEdge the edge is for JSString or JSObject.

No, I was hoping to not have a dynamic record of this at all. There would be two separate tables, so you statically know what type you're tracing and don't have to do any dynamic checks at all.

I'm not sure what the best way to implement this is. I suppose you could templatize CellPtrEdge so you'd have CellPtrEdge<JSObject> and CellPtrEdge<JSString>. But I don't think that's necessary; I think you can just have https://searchfox.org/mozilla-central/source/js/src/gc/StoreBuffer.h#477 call objectBufferCell.traceTyped<JSObject>(mover); stringBufferCell.traceTyped<JSString>(mover);. At least, I don't think it's a problem to define a templatized traceTyped that is never instantiated for other MonoTypeBuffer<T> instantiations.

And for 'only check IsInsideNursery once.'
https://searchfox.org/mozilla-central/rev/f4c39907e0b527dc4b9356a1eeb8c6e6c62d383a/js/src/gc/Marking.cpp#2792

Do you mean that because we already check if the cell is in nursery in maybeInRemberedSet
https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/StoreBuffer.h#255

I wasn't looking at that one. That happens when putting it into the store buffer, I'm not sure it's guaranteed to not have been updated to a tenured location by the time we hit that trace line?

So we can remove the IsInsideNursery here?
https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/Marking.cpp#2794

The redundant one I was referring to is inside mover.traverse<JSString*>: https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/Marking.cpp#2644

So I thought I remembered that one of the JSObject and JSString cases did something different, which made it hard to remove the check. But it looks like they both do the check first thing.

So actually, I think this part could be a minor fix independent of splitting out a JSString-only cell store buffer. I was trying to eliminate https://searchfox.org/mozilla-central/rev/f4c39907e0b527dc4b9356a1eeb8c6e6c62d383a/js/src/gc/Marking.cpp#2794 but that has to happen before the nurseryCellIsString check, so it's not easy to remove. But I see now that there are no other callers of either mover.traverse<T>, so both of https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/Marking.cpp#2623,2639 could be converted to MOZ_ASSERTs with no other changes.

Comment 5

[Steve Fink [:sfink] [:s:]]

I expect the tricky part will be in routing JIT storebuffer insertions to the correct storebuffer. And if that's too difficult (or slow, or emits a bunch more code), then perhaps it's not worth doing this bug at all.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)
Thanks for the detail explanation.

But I see now that there are no other callers of either mover.traverse<T>, so both of https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/Marking.cpp#2623,2639 could be converted to MOZ_ASSERTs with no other changes.

just to confirm, you mean
https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/js/src/gc/Marking.cpp#2623,2644 ?

And I found for TenuringTracer::traverse<JSObject> and TenuringTracer::traverse<JSString> will also be called from TenuringTracer::traverse<JS::Value>, (which is called by js::TenuringTracer::traceSlots), and Value will be converted to JSObject or JSString by MapGCThingTyped
https://searchfox.org/mozilla-central/source/js/public/Value.h#1315,1320
(rev: 6c9f60f8cc064a1005cd8141ecd526578ae9da7a)

I'll check if replacing MOZ_ASSERT is still okay.
Thanks

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1376646 - Part 1: Separate buffer for storing JSString and JSObject cells.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1376646 - Part 2: remove redudant IsInsideNursery() check. r?sfink

TenuringTracer::traverse(JSObject**) and
TenuringTracer::traverse(JSString**) will check IsInsideNursery(),
and also these two functions will be called by other callers,
so removing the check in CellPtrEdge::traceTyped

Depends on D32097

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 9066615 [details]
Bug 1376646 - Part 1: Separate buffer for storing JSString and JSObject cells.

Hi Steve
could you check if this is what you have in mind?

I am still figuring out the JIT part, will check if I can fix in this bug.

Thanks

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 9066758 [details]
Bug 1376646 - Part 2: remove redudant IsInsideNursery() check. r?sfink

Given it still has other callers, and TenuingTracer will check if it's in the nursery, so I simply remove the check. Does this make sense?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b5401dd492
Part 1: Separate buffer for storing JSString and JSObject cells. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/b89b3cda594a
Part 2: remove redudant IsInsideNursery() check. r=sfink

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/67b5401dd492
https://hg.mozilla.org/mozilla-central/rev/b89b3cda594a