Bug 162354 – Need a store-load fence between setting cell state and visiting the object in SlotVisitor

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 289471 [details]
something like this

[Filip Pizlo]

Created attachment 289480 [details]
looks like it passes some tests

[Filip Pizlo]

HOLY COW!  This totally removes the cost of the mfence.

[Filip Pizlo]

(In reply to comment #3)
> HOLY COW!  This totally removes the cost of the mfence.

... in Octane/splay.  Still need to test other things.

[Filip Pizlo]

Created attachment 289485 [details]
the patch

[Geoffrey Garen]

Comment on attachment 289485 [details]
the patch

r=me

[Filip Pizlo]

Comment on attachment 289485 [details]
the patch

Aw crap, after testing it more, I'm seeing a splay regression.  I'll spend more time on this.

[Filip Pizlo]

Created attachment 289644 [details]
the patch

[Filip Pizlo]

Created attachment 289646 [details]
the patch

[Filip Pizlo]

Created attachment 289649 [details]
the patch

[Build Bot]

Comment on attachment 289649 [details]
the patch

Attachment 289649 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2129415

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289654 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289649 [details]
the patch

Attachment 289649 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2129425

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289655 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Created attachment 289657 [details]
the patch

For real this time.  The last patch was DOA because I used #elsif instead of #elif. D'oh.

[JF Bastien]

Comment on attachment 289657 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289657&action=review

> Source/JavaScriptCore/heap/HeapInlines.h:128
> +    if (!from || static_cast<unsigned>(from->cellState()) > blackThreshold)

Isn't a global comparison operator next to the enum easier to do?

[Filip Pizlo]

(In reply to comment #16)
> Comment on attachment 289657 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289657&action=review
> 
> > Source/JavaScriptCore/heap/HeapInlines.h:128
> > +    if (!from || static_cast<unsigned>(from->cellState()) > blackThreshold)
> 
> Isn't a global comparison operator next to the enum easier to do?

Can you show me an example?

[Mark Lam]

Comment on attachment 289657 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289657&action=review

r=me with suggestions.

> Source/JavaScriptCore/heap/Heap.cpp:978
> -    SuperSamplerScope superSamplerScope(false);
> +    SuperSamplerScope superSamplerScope(true);

Please undo if this is not necessary.

>> Source/JavaScriptCore/heap/HeapInlines.h:128
>> +    if (!from || static_cast<unsigned>(from->cellState()) > blackThreshold)
> 
> Isn't a global comparison operator next to the enum easier to do?

I was thinking something like !isBlack(from->cellState()) would be more clear about the intent of this expression.  I understand that the JIT and LLInt code can't use a helper isBlack() function, but at least C++ code reads better.  Plus it expresses your comment in CellState.h in code form.  What do you think?

> Source/JavaScriptCore/heap/SlotVisitor.cpp:305
> +    // FIXME: Make this work on ARM also.
> +    // https://bugs.webkit.org/show_bug.cgi?id=162461
> +    if (isX86())
> +        WTF::storeLoadFence();

Why would this not work on ARM as well?  It has a storeLoadFence() (perf aside), no?

[Filip Pizlo]

(In reply to comment #18)
> Comment on attachment 289657 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289657&action=review
> 
> r=me with suggestions.
> 
> > Source/JavaScriptCore/heap/Heap.cpp:978
> > -    SuperSamplerScope superSamplerScope(false);
> > +    SuperSamplerScope superSamplerScope(true);
> 
> Please undo if this is not necessary.

Undone.

> 
> >> Source/JavaScriptCore/heap/HeapInlines.h:128
> >> +    if (!from || static_cast<unsigned>(from->cellState()) > blackThreshold)
> > 
> > Isn't a global comparison operator next to the enum easier to do?
> 
> I was thinking something like !isBlack(from->cellState()) would be more
> clear about the intent of this expression.  I understand that the JIT and
> LLInt code can't use a helper isBlack() function, but at least C++ code
> reads better.  Plus it expresses your comment in CellState.h in code form. 
> What do you think?

That's perfect.  I added bool isBlack(CellState) and use it throughout.

> 
> > Source/JavaScriptCore/heap/SlotVisitor.cpp:305
> > +    // FIXME: Make this work on ARM also.
> > +    // https://bugs.webkit.org/show_bug.cgi?id=162461
> > +    if (isX86())
> > +        WTF::storeLoadFence();
> 
> Why would this not work on ARM as well?  It has a storeLoadFence() (perf
> aside), no?

Just perf.  In a separate patch, I want to experiment with the best store-load fence strategies on ARM, and I want to find out if this is possible on ARM at all.

[Filip Pizlo]

Created attachment 289690 [details]
patch for landing

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/206314

[Csaba Osztrogonác]

(In reply to comment #21)
> Landed in https://trac.webkit.org/changeset/206314

It broke the cloop build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/10018

[Filip Pizlo]

(In reply to comment #22)
> (In reply to comment #21)
> > Landed in https://trac.webkit.org/changeset/206314
> 
> It broke the cloop build:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/10018

I'm on it.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 162506

[Filip Pizlo]

Created attachment 289711 [details]
patch for relanding

[Filip Pizlo]

Created attachment 289712 [details]
patch for relanding

And now, with a changelog!

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/206344