Closed Bug 1175642 Opened 9 years ago Closed 9 years ago

Remove the duplicate storebuffer insertion APIs

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

With bug 1174850, postBarrier and postBarrierRelocatable look almost identical, so let's common them up. I've chosen to make Insert and Remove symmetrical, which I think works well, now that all post-barriers are relocatable.
Attached patch remove_duplicate_code-v0.diff (obsolete) — Splinter Review
Note, this is largely split off from bug 1174873 -- mostly just name changes from what you reviewed there. I wanted to get more of the groundwork out of the way separately to make backout easier, if needed.
Attachment #8623835 - Flags: review?(jcoppeard)
Comment on attachment 8623835 [details] [diff] [review]
remove_duplicate_code-v0.diff

After yet another day of struggling with getting this to actually work in practice, I've decided that we need to fix the API first. The problem this time is that merging Heap and Relocatable means that sometimes ~RelocatablePtr can get killed after the target has already been collected so that our checks for isNeeded(prior) will explode.

I think what we want here is actually more like |StoreBuffer::freshen(addr, prior, new)| that just grabs the table row and updates it as needed.
Attachment #8623835 - Flags: review?(jcoppeard)
Well, that's not quite what we need, but pretty close. What we currently have is:

        if (InternalGCMethods<T>::needsPostBarrier(v)) {
            this->value = v;
            post();
        } else if (InternalGCMethods<T>::needsPostBarrier(this->value)) {
            relocate();
            this->value = v;
        } else {
            this->value = v;
        }

Doing a case-by-case analysis, we can see that this has performance like:

        // nullptr -> nullptr => 2 null check
        // nullptr -> tenured => 2 null check; 1 sb lookup
        // nullptr -> nursery => 1 null check; 1 sb lookup; 1 insert

        // tenured -> nullptr => 2 null check; 1 sb lookup;
        // tenured -> tenured => 2 null check; 2 sb lookup;
        // tenured -> nursery => 1 null check; 2 sb lookup; 1 insert

        // nursery -> nullptr => 2 null check; 1 sb lookup; 1 remove
        // nursery -> tenured => 2 null check; 2 sb lookup; 1 remove
        // nursery -> nursery => 1 null check; 1 sb lookup; 1 touch

Not bad at all! Probably even optimal. However, the reason we have needsPostBarrier at all, as well as total lack of generality elsewhere is specifically so that I could get this ::set implementation to have minimal operations with only a mildly confusing body. But given that this probably isn't all that hot, can we do better?

I think so; let's make the ::set do:

        T tmp = this->value;
        this->value = v;
        post(tmp, this->value);

It requires an extra reg for tmp, but this is probably going to be optimized out. The only reason we need the tmp at all is that we assert that *&value is the current value deep in the store buffer -- we could relax this easily. Then the actual implementation becomes:

    if (next && (auto buffer = next->storeBuffer())) {
        if (prev && prev->storeBuffer())
            return; // we already have the entry
        buffer->putCellFromAnyThread(objp);
        return;
    }
    if (prev && (auto buffer = prev->storeBuffer()))
        buffer->unputCellFromAnyThread(objp);

I'm fairly certain that given general prev/next, this is equivalent to the above. Our constructor and destructor barriers will be a bit heavier, but because the nullptr is constant, the compiler should be able to optimize this down to the same code as before.

The real advantage here is that we can get rid of needsPostBarrier and postBarrierRemove, bringing us down to a single postBarrier implementation function. I didn't have time to test much tonight, so this might all be nonsense, but I think this should be workable in some form.
No longer depends on: 1174850
Seems to work locally. Try run is at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c577077d2f61

 11 files changed, 95 insertions(+), 210 deletions(-)

110 of the most obscure lines in SM gone.
Attachment #8623835 - Attachment is obsolete: true
Attachment #8624454 - Flags: review?(jcoppeard)
Comment on attachment 8624454 [details] [diff] [review]
fix_the_relocatable_iterface-v0.diff

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

::: dom/xbl/nsXBLMaybeCompiled.h
@@ +100,5 @@
> +      Base::postBarrier(&functionp->UnsafeGetJSFunction(),
> +                        prev.IsCompiled() ? prev.UnsafeGetJSFunction() : nullptr,
> +                        next.UnsafeGetJSFunction());
> +    } else if (prev.IsCompiled()) {
> +      Base::postBarrier(&prev.UnsafeGetJSFunction(),

Is this correct?  I looks like it should be &functionp->UnsafeGetJSFunction() in both cases.

I think it might be better to add MaybeGetJSFunction() to nsXBLMaybeCompiled so you can handle both cases with something like:

  Base::postBarrier(&functionp->UnsafeGetJSFunction(), prev.MaybeGetJSFunction(), next.MaybeGetJSFunction())

::: js/src/gc/Barrier.h
@@ +272,5 @@
> +        if (next.isObject() && (sb = reinterpret_cast<gc::Cell*>(&next.toObject())->storeBuffer())) {
> +            // If we know that the prev has already inserted an entry, we can skip
> +            // doing the lookup to add the new entry.
> +            if (prev.isObject() && reinterpret_cast<gc::Cell*>(&prev.toObject())->storeBuffer()) {
> +                //MOZ_ASSERT(buffer->hasCellFromAnyThread(cellp));

Should be uncommented or removed.  Oh maybe you didn't implement hasCellFromAnyThread() yet?

::: js/src/jsobj.h
@@ +618,5 @@
> +    if (!IsNullTaggedPointer(next) && (buffer = next->storeBuffer())) {
> +        // If we know that the prev has already inserted an entry, we can skip
> +        // doing the lookup to add the new entry.
> +        if (!IsNullTaggedPointer(prev) && prev->storeBuffer()) {
> +            //MOZ_ASSERT(buffer->hasCellFromAnyThread(cellp));

Ditto.

::: js/src/vm/Stack.cpp
@@ -107,5 @@
>  #endif
>  }
>  
> -void
> -InterpreterFrame::writeBarrierPost()

Oh I guess this doesn't get called.
Attachment #8624454 - Flags: review?(jcoppeard) → review+
functionp is the same as next, so if !next.IsCompiled(), the address in next might be garbage, or at least useless to us for the purposes of removing the entry.

In particular, take the case where we're deleting a compiled function: we want to remove any entry that has been placed on it, but the value in next here is going to be the temporary returned by GCMethods<...>::initial(). The address of this temporary isn't relevant, it's the address of prev's compiled function that we want to remove.
Landed on mozilla-central this morning but didn't got marked for some reason.
https://hg.mozilla.org/mozilla-central/rev/6b847a10bbb1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Backed out for now. Bug 1174319 seems to have regressed octane and I don't have time to track this down while paying attention to work-week presentations.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Let's reland the less-likely regressor to see what happens.
It was indeed merge bustage. I took out the patch removing the pre-buffer from the store buffer, so |has| now has to call sinkStores.
https://hg.mozilla.org/mozilla-central/rev/3c61b61ea4a2
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 1207400
You need to log in before you can comment on or make changes to this bug.