Patch forthcoming.
Created attachment 289471 [details] something like this
Created attachment 289480 [details] looks like it passes some tests
HOLY COW! This totally removes the cost of the mfence.
(In reply to comment #3) > HOLY COW! This totally removes the cost of the mfence. ... in Octane/splay. Still need to test other things.
Created attachment 289485 [details] the patch
Comment on attachment 289485 [details] the patch r=me
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.
Created attachment 289644 [details] the patch
Created attachment 289646 [details] the patch
Created attachment 289649 [details] the patch
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.
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 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.
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
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 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?
(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 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?
(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.
Created attachment 289690 [details] patch for landing
Landed in https://trac.webkit.org/changeset/206314
(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
(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.
Re-opened since this is blocked by bug 162506
Created attachment 289711 [details] patch for relanding
Created attachment 289712 [details] patch for relanding And now, with a changelog!
Landed in https://trac.webkit.org/changeset/206344