Bug 162354

Summary: Need a store-load fence between setting cell state and visiting the object in SlotVisitor
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, keith_miller, mark.lam, msaboff, ossy, rniwa, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 162506    
Bug Blocks: 162316    
Attachments:
Description Flags
something like this
none
looks like it passes some tests
none
the patch
fpizlo: review-
the patch
none
the patch
none
the patch
buildbot: commit-queue-
Archive of layout-test-results from ews116 for mac-yosemite
none
Archive of layout-test-results from ews102 for mac-yosemite
none
the patch
mark.lam: review+
patch for landing
none
patch for relanding
none
patch for relanding none

Description Filip Pizlo 2016-09-21 12:35:00 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-09-21 12:35:50 PDT
Created attachment 289471 [details]
something like this
Comment 2 Filip Pizlo 2016-09-21 15:18:37 PDT
Created attachment 289480 [details]
looks like it passes some tests
Comment 3 Filip Pizlo 2016-09-21 15:24:59 PDT
HOLY COW!  This totally removes the cost of the mfence.
Comment 4 Filip Pizlo 2016-09-21 15:25:45 PDT
(In reply to comment #3)
> HOLY COW!  This totally removes the cost of the mfence.

... in Octane/splay.  Still need to test other things.
Comment 5 Filip Pizlo 2016-09-21 15:33:02 PDT
Created attachment 289485 [details]
the patch
Comment 6 Geoffrey Garen 2016-09-21 15:49:19 PDT
Comment on attachment 289485 [details]
the patch

r=me
Comment 7 Filip Pizlo 2016-09-21 15:57:46 PDT
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.
Comment 8 Filip Pizlo 2016-09-22 18:41:36 PDT
Created attachment 289644 [details]
the patch
Comment 9 Filip Pizlo 2016-09-22 19:00:52 PDT
Created attachment 289646 [details]
the patch
Comment 10 Filip Pizlo 2016-09-22 19:17:36 PDT
Created attachment 289649 [details]
the patch
Comment 11 Build Bot 2016-09-22 20:12:06 PDT
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.
Comment 12 Build Bot 2016-09-22 20:12:11 PDT
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
Comment 13 Build Bot 2016-09-22 20:15:06 PDT
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.
Comment 14 Build Bot 2016-09-22 20:15:12 PDT
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
Comment 15 Filip Pizlo 2016-09-22 20:27:54 PDT
Created attachment 289657 [details]
the patch

For real this time.  The last patch was DOA because I used #elsif instead of #elif. D'oh.
Comment 16 JF Bastien 2016-09-23 10:10:23 PDT
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?
Comment 17 Filip Pizlo 2016-09-23 10:18:58 PDT
(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?
Comment 18 Mark Lam 2016-09-23 10:29:15 PDT
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?
Comment 19 Filip Pizlo 2016-09-23 10:32:30 PDT
(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.
Comment 20 Filip Pizlo 2016-09-23 10:52:39 PDT
Created attachment 289690 [details]
patch for landing
Comment 21 Filip Pizlo 2016-09-23 11:12:21 PDT
Landed in https://trac.webkit.org/changeset/206314
Comment 23 Filip Pizlo 2016-09-23 11:38:27 PDT
(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.
Comment 24 WebKit Commit Bot 2016-09-23 13:17:35 PDT
Re-opened since this is blocked by bug 162506
Comment 25 Filip Pizlo 2016-09-23 14:56:32 PDT
Created attachment 289711 [details]
patch for relanding
Comment 26 Filip Pizlo 2016-09-23 14:57:32 PDT
Created attachment 289712 [details]
patch for relanding

And now, with a changelog!
Comment 27 Filip Pizlo 2016-09-23 17:50:58 PDT
Landed in https://trac.webkit.org/changeset/206344