WebKit Bugzilla
Attachment 343628 Details for
Bug 187059
: REGRESSION(r233184): "It regressed JetStream between 5-8%" (Requested by saamyjoon on #webkit).
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
ROLLOUT of r233184
bug-187059-20180626150159.patch (text/plain), 17.50 KB, created by
WebKit Commit Bot
on 2018-06-26 12:02:00 PDT
(
hide
)
Description:
ROLLOUT of r233184
Filename:
MIME Type:
Creator:
WebKit Commit Bot
Created:
2018-06-26 12:02:00 PDT
Size:
17.50 KB
patch
obsolete
>Subversion Revision: 233211 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 0458c56f270541a87cf5be836ee20756fcc26de3..eec1c399255555eb16af833e7e22a397f3b2d27f 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,18 @@ >+2018-06-26 Commit Queue <commit-queue@webkit.org> >+ >+ Unreviewed, rolling out r233184. >+ https://bugs.webkit.org/show_bug.cgi?id=187059 >+ >+ "It regressed JetStream between 5-8%" (Requested by saamyjoon >+ on #webkit). >+ >+ Reverted changeset: >+ >+ "JSImmutableButterfly can't be allocated from a subspace with >+ HeapCell::Kind::Auxiliary" >+ https://bugs.webkit.org/show_bug.cgi?id=186878 >+ https://trac.webkit.org/changeset/233184 >+ > 2018-06-26 Carlos Alberto Lopez Perez <clopez@igalia.com> > > REGRESSION(r233065): Build broken with clang-3.8 and libstdc++-5 >diff --git a/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp b/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp >index 850b3d204d3826d9e140034104a94efc3bc0f48e..0c909d0771d62f7586f4f0cd6457c9637e26444a 100644 >--- a/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp >+++ b/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp >@@ -406,7 +406,6 @@ RegisterID* ArrayNode::emitBytecode(BytecodeGenerator& generator, RegisterID* ds > auto newArray = [&] (RegisterID* dst, ElementNode* elements, unsigned length, bool hadVariableExpression) { > if (length && !hadVariableExpression) { > recommendedIndexingType |= CopyOnWrite; >- ASSERT(generator.vm()->heap.isDeferred()); // We run bytecode generator under a DeferGC. If we stopped doing that, we'd need to put a DeferGC here as we filled in these slots. > auto* array = JSImmutableButterfly::create(*generator.vm(), recommendedIndexingType, length); > unsigned index = 0; > for (ElementNode* element = elements; index < length; element = element->next()) { >diff --git a/Source/JavaScriptCore/heap/HeapUtil.h b/Source/JavaScriptCore/heap/HeapUtil.h >index dd6d8de33e1867080b5571365d2c782227291781..01cfdbca03fe32a8f06e2e0478688673761b732e 100644 >--- a/Source/JavaScriptCore/heap/HeapUtil.h >+++ b/Source/JavaScriptCore/heap/HeapUtil.h >@@ -87,7 +87,8 @@ public: > char* previousPointer = pointer - sizeof(IndexingHeader) - 1; > MarkedBlock* previousCandidate = MarkedBlock::blockFor(previousPointer); > if (!filter.ruleOut(bitwise_cast<Bits>(previousCandidate)) >- && set.contains(previousCandidate)) { >+ && set.contains(previousCandidate) >+ && previousCandidate->handle().cellKind() == HeapCell::Auxiliary) { > previousPointer = static_cast<char*>(previousCandidate->handle().cellAlign(previousPointer)); > if (previousCandidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, previousPointer)) > func(previousPointer, previousCandidate->handle().cellKind()); >@@ -102,16 +103,23 @@ public: > if (!set.contains(candidate)) > return; > >- MarkedBlock::Handle& handle = candidate->handle(); >- HeapCell::Kind cellKind = handle.cellKind(); >+ HeapCell::Kind cellKind = candidate->handle().cellKind(); > > auto tryPointer = [&] (void* pointer) { >- if (handle.isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer)) >+ if (candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer)) > func(pointer, cellKind); > }; > >+ if (candidate->handle().cellKind() == HeapCell::JSCell) { >+ if (!MarkedBlock::isAtomAligned(pointer)) >+ return; >+ >+ tryPointer(pointer); >+ return; >+ } >+ > // A butterfly could point into the middle of an object. >- char* alignedPointer = static_cast<char*>(handle.cellAlign(pointer)); >+ char* alignedPointer = static_cast<char*>(candidate->handle().cellAlign(pointer)); > tryPointer(alignedPointer); > > // Also, a butterfly could point at the end of an object plus sizeof(IndexingHeader). In that >diff --git a/Source/JavaScriptCore/runtime/JSImmutableButterfly.h b/Source/JavaScriptCore/runtime/JSImmutableButterfly.h >index e5fcc181b8ca7b6ab1e814356bea39f94af08e49..3ace0ad3003983980297d7f025035acbd887ceeb 100644 >--- a/Source/JavaScriptCore/runtime/JSImmutableButterfly.h >+++ b/Source/JavaScriptCore/runtime/JSImmutableButterfly.h >@@ -89,7 +89,7 @@ public: > static CompleteSubspace* subspaceFor(VM& vm) > { > // We allocate out of the JSValue gigacage as other code expects all butterflies to live there. >- return &vm.jsValueGigacageCellSpace; >+ return &vm.jsValueGigacageAuxiliarySpace; > } > > // Only call this if you just allocated this butterfly. >diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index dece0a4273f8a3d4fd3910a4957aa2f0ef41f90b..354a05bb5fedb710a6c77c8091c52f4aeb577f31 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -1,3 +1,18 @@ >+2018-06-26 Commit Queue <commit-queue@webkit.org> >+ >+ Unreviewed, rolling out r233184. >+ https://bugs.webkit.org/show_bug.cgi?id=187059 >+ >+ "It regressed JetStream between 5-8%" (Requested by saamyjoon >+ on #webkit). >+ >+ Reverted changeset: >+ >+ "JSImmutableButterfly can't be allocated from a subspace with >+ HeapCell::Kind::Auxiliary" >+ https://bugs.webkit.org/show_bug.cgi?id=186878 >+ https://trac.webkit.org/changeset/233184 >+ > 2018-06-26 Truitt Savell <tsavell@apple.com> > > Layout Test http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction.html is flaky >diff --git a/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt b/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt >index 369e27c8b1dd453723750b85cc5b95c9891ca057..b40cca594cf7b1859a733d80e95e5873d70c3883 100644 >--- a/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt >+++ b/LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt >@@ -3,7 +3,10 @@ This tests navigating away from a document after setting a selection deletes the > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > > >-PASS freed more than 60 documents >+iframe = appendIframe() >+PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1 >+iframe.src = "resources/select-iframe-focusin-document-crash-frame.html" >+PASS internals.numberOfLiveDocuments() is initialDocumentCount + 1 > PASS successfullyParsed is true > > TEST COMPLETE >diff --git a/LayoutTests/editing/selection/navigation-clears-editor-state.html b/LayoutTests/editing/selection/navigation-clears-editor-state.html >index d3e0952ce4f258f186499abbd22f507c42b7f541..a0ee4fb31b6f510d1088ac5bb38a574bb15b01e3 100644 >--- a/LayoutTests/editing/selection/navigation-clears-editor-state.html >+++ b/LayoutTests/editing/selection/navigation-clears-editor-state.html >@@ -35,36 +35,21 @@ async function runTest() > { > initialDocumentCount = internals.numberOfLiveDocuments(); > >- let frames = []; >- const count = 200; >- for (let i = 0; i < count; ++i) >- frames.push(appendIframe()); >+ evalAndLog('iframe = appendIframe()'); > > await wait(0); // Make sure the transient document created by inserting an iframe is removed. > GCController.collect(); > >- for (let frame of frames) >- setEditorStates(frame); >+ shouldBe('internals.numberOfLiveDocuments()', 'initialDocumentCount + 1'); >+ setEditorStates(iframe); > > await wait(0); // Wait for UI update timer to fire. > >- let resolved = 0; >- for (let frame of frames) { >- frame.src = "resources/select-iframe-focusin-document-crash-frame.html"; >- frame.onload = () => { >- ++resolved; >- if (resolved !== count) >- return; >- let before = internals.numberOfLiveDocuments(); >- GCController.collect(); >- let after = internals.numberOfLiveDocuments(); >- let delta = before - after; >- if (delta > 0.3 * count) >- debug("PASS freed more than 60 documents"); >- else >- debug("FAIL freed fewer than 60 documents"); >- finishJSTest(); >- }; >+ evalAndLog('iframe.src = "resources/select-iframe-focusin-document-crash-frame.html"'); >+ iframe.onload = () => { >+ GCController.collect(); >+ shouldBe('internals.numberOfLiveDocuments()', 'initialDocumentCount + 1'); >+ finishJSTest(); > } > } > >@@ -72,7 +57,7 @@ if (!window.GCController || !window.internals) { > debug('This test requires GCController and internals'); > } else { > if (window.testRunner) >- setTimeout(() => testRunner.notifyDone(), 5000); >+ setTimeout(() => testRunner.notifyDone(), 3000); > // Clear out any lingering documents from the previous tests. > GCController.collect(); > GCController.collect(); >diff --git a/LayoutTests/fast/dom/reference-cycle-leaks.html b/LayoutTests/fast/dom/reference-cycle-leaks.html >index e69b2a3540736a98482874ced89d2d948859d5dd..0953d57d68edbfe463c3e228dd56050935d82aa7 100644 >--- a/LayoutTests/fast/dom/reference-cycle-leaks.html >+++ b/LayoutTests/fast/dom/reference-cycle-leaks.html >@@ -7,7 +7,7 @@ description('Tests for leaks caused by reference cycles that pass through the DO > function checkForNodeLeaks(testFunction, underlyingClass) > { > // Bump this number as high as we need to, to get reproducible results. >- const repetitions = 40; >+ const repetitions = 20; > > gc(); > const beforeCount = internals.numberOfLiveNodes(); >diff --git a/LayoutTests/fast/misc/resources/test-observegc.js b/LayoutTests/fast/misc/resources/test-observegc.js >index dc27d7bc1c9c7437599d83cf23d41bd31518455a..65e68f509d78011da0b76a797b3f681dd50cb543 100644 >--- a/LayoutTests/fast/misc/resources/test-observegc.js >+++ b/LayoutTests/fast/misc/resources/test-observegc.js >@@ -1,19 +1,9 @@ > description("Ensures that window.internals.observegc works as expected"); > >-var observers = []; >-for (let i = 0; i < 1000; ++i) { >- let testObject = { testProperty : "testValue" }; >- let observer = internals.observeGC(testObject); >- observers.push(observer); >- testObject = null; >-} >+var testObject = { testProperty : "testValue" }; > >+var observer = internals.observeGC(testObject); >+testObject = null; > gc(); > >-var anyCollected = false; >-for (let observer of observers) { >- if (observer.wasCollected) >- anyCollected = true; >-} >- >-shouldBe('anyCollected', 'true'); >+shouldBe('observer.wasCollected', 'true'); >diff --git a/LayoutTests/fast/misc/test-observegc-expected.txt b/LayoutTests/fast/misc/test-observegc-expected.txt >index 808d13cdcc6c13475fce36476ebbafff4da862b4..c9edc874b5d335b33f26ad28a657c1e0a452be48 100644 >--- a/LayoutTests/fast/misc/test-observegc-expected.txt >+++ b/LayoutTests/fast/misc/test-observegc-expected.txt >@@ -3,7 +3,7 @@ Ensures that window.internals.observegc works as expected > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > > >-PASS anyCollected is true >+PASS observer.wasCollected is true > PASS successfullyParsed is true > > TEST COMPLETE >diff --git a/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt b/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt >index 6f752dcc015bd67f65e7399301fd557025983bf0..8c6dc35189b3b02bbef8503887b0d755b754dd18 100644 >--- a/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt >+++ b/LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt >@@ -1,8 +1,12 @@ >-PASS countAfterCreate === countOrig + 50 is true >-FAIL countAfterGC < countAfterCreate should be true. Was false. >-PASS refAfterGet === refOrig + 50 is true >-FAIL refAfterGetGC < refAfterGet should be true. Was false. >-PASS refAfterPass === refBeforePass + 50 is true >-FAIL refAfterPassAndGC < refAfterPass should be true. Was false. > Test that we can get an NPObject returned through a method on an NPAPI Object. >+Prints "SUCCESS" on success, "FAILURE" on failure. > >+--- num test objects: >+countAfterCreate == countOrig + 3? PASS >+countOrig == countAfterGC? FAIL >+ >+--- refcount on plug.testObject: >+originally: 2 >+after GC: 5 >+after passing: 8 >+FAILURE >diff --git a/LayoutTests/plugins/refcount-leaks-expected.txt b/LayoutTests/plugins/refcount-leaks-expected.txt >index 9e35f693274af1984d1ef9d08e1014957bc02c0f..d51a342e035a83933826c7dd60ebb8880a6a7a7b 100644 >--- a/LayoutTests/plugins/refcount-leaks-expected.txt >+++ b/LayoutTests/plugins/refcount-leaks-expected.txt >@@ -1,8 +1,12 @@ >-PASS countAfterCreate === countOrig + 50 is true >-PASS countAfterGC < countAfterCreate is true >-PASS refAfterGet === refOrig + 50 is true >-PASS refAfterGetGC < refAfterGet is true >-PASS refAfterPass === refBeforePass + 50 is true >-PASS refAfterPassAndGC < refAfterPass is true > Test that we can get an NPObject returned through a method on an NPAPI Object. >+Prints "SUCCESS" on success, "FAILURE" on failure. > >+--- num test objects: >+countAfterCreate == countOrig + 3? PASS >+countOrig == countAfterGC? PASS >+ >+--- refcount on plug.testObject: >+originally: 2 >+after GC: 2 >+after passing: 2 >+SUCCESS >diff --git a/LayoutTests/plugins/refcount-leaks.html b/LayoutTests/plugins/refcount-leaks.html >index 0eb4d7942c544f3c4f58ef44aa2142420196eff9..69f6ee0a5af5757d24d1f55e0954f759cb140d91 100644 >--- a/LayoutTests/plugins/refcount-leaks.html >+++ b/LayoutTests/plugins/refcount-leaks.html >@@ -1,74 +1,83 @@ >-<head> >-<script src="../resources/js-test-pre.js"></script> >-</head> >- > <script> >-function noop(x) { >-} >+ function noop(x) { >+ } > >-function doGC() { >- // GC twice to make sure everything is cleaned up. >- for (var i = 0; i < 2; i++) { >- gc(); >+ function doGC() { >+ if (window.GCController) { >+ // GC twice to make sure everything is cleaned up. >+ for (var i = 0; i < 2; i++) { >+ window.GCController.collect(); >+ } > } >-} >- >-var countOrig; >-var countAfterCreate; >-var countAfterGC; >-var testObj; >-var refOrig; >-var refAfterGet; >-var refAfterGetGC; >-var refBeforePass; >-var refAfterPass; >-var refAfterPassAndGC; >- >-function runtest() { >+ } >+ >+ function runtest() { > if (window.testRunner) >- testRunner.dumpAsText(); >+ testRunner.dumpAsText(); >+ >+ >+ var output = document.getElementById("output"); >+ output.innerHTML = ""; > > // Test that objects are deleted after their JS references are released. >- countOrig = plug.testObjectCount; >- let x; >- for (let i = 0; i < 50; ++i) >- x = plug.testCreateTestObject(); >- countAfterCreate = plug.testObjectCount; >- x = null; >+ var countOrig = plug.testObjectCount; >+ o1 = plug.testCreateTestObject(); >+ o2 = plug.testCreateTestObject(); >+ o3 = plug.testCreateTestObject(); >+ var countAfterCreate = plug.testObjectCount; >+ o1 = o2 = o3 = null; > doGC(); >- countAfterGC = plug.testObjectCount; >- shouldBe('countAfterCreate === countOrig + 50', 'true'); >- shouldBe('countAfterGC < countAfterCreate', 'true'); >+ var countAfterGC = plug.testObjectCount; >+ >+ output.innerHTML += "--- num test objects:<br>"; >+ output.innerHTML += "countAfterCreate == countOrig + 3? " >+ + ((countAfterCreate == countOrig + 3) ? "PASS" : "FAIL") >+ + "<br>"; >+ output.innerHTML += "countOrig == countAfterGC? " >+ + ((countOrig == countAfterGC) ? "PASS" : "FAIL") >+ + "<br>"; >+ output.innerHTML += "<br>"; > > // Test that the object refcount returns to normal after JS references > // are released. >- testObj = plug.testObject; >- refOrig = testObj.refCount; >- for (let i = 0; i < 50; ++i) >- plug.testObject; >- refAfterGet = testObj.refCount; >+ var testObj = plug.testObject; >+ var refOrig = testObj.refCount; >+ var o1 = plug.testObject; >+ var o2 = plug.testObject; >+ var o3 = plug.testObject; >+ var refAfterGet = testObj.refCount; >+ o1 = o2 = o3 = null; > doGC(); >- refAfterGetGC = testObj.refCount; >- shouldBe('refAfterGet === refOrig + 50', 'true'); >- shouldBe('refAfterGetGC < refAfterGet', 'true'); >+ var refAfterGetGC = testObj.refCount; > > // Test that calling NPN_Invoke with our object as a parameter returns > // our refcount to normal (may require a GC). >- refBeforePass = testObj.refCount; >- for (let i = 0; i < 50; ++i) >- plug.testPassTestObject("noop", testObj); >- refAfterPass = testObj.refCount; >+ plug.testPassTestObject("noop", testObj); >+ plug.testPassTestObject("noop", testObj); >+ plug.testPassTestObject("noop", testObj); > doGC(); >- refAfterPassAndGC = testObj.refCount; >- shouldBe('refAfterPass === refBeforePass + 50', 'true'); >- shouldBe('refAfterPassAndGC < refAfterPass', 'true'); >-} >+ var refAfterPass = testObj.refCount; >+ >+ output.innerHTML += "--- refcount on plug.testObject:<br>"; >+ output.innerHTML += "originally: " + refOrig + "<br>"; >+ output.innerHTML += "after GC: " + refAfterGetGC + "<br>"; >+ output.innerHTML += "after passing: " + refAfterPass + "<br>"; >+ >+ var success = (countAfterGC == countOrig) && (refAfterPass == refOrig); >+ output.innerHTML += (success ? "SUCCESS" : "FAILURE"); >+ } > </script> > > <body onload="runtest()"> > >- Test that we can get an NPObject returned through a method on >- an NPAPI Object.<P> >+Test that we can get an NPObject returned through a method on >+an NPAPI Object.<P> >+ >+Prints "SUCCESS" on success, "FAILURE" on failure. >+ >+<embed name="plug" type="application/x-webkit-test-netscape"> >+ >+<div id=output>FAILURE</div> > >- <embed name="plug" type="application/x-webkit-test-netscape"> > </body> >+
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=343628">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 187059
: 343628