Unreviewed, rolling out r233184.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Jun 2018 19:14:07 +0000 (19:14 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Jun 2018 19:14:07 +0000 (19:14 +0000)
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

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233213 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/selection/navigation-clears-editor-state-expected.txt
LayoutTests/editing/selection/navigation-clears-editor-state.html
LayoutTests/fast/dom/reference-cycle-leaks.html
LayoutTests/fast/misc/resources/test-observegc.js
LayoutTests/fast/misc/test-observegc-expected.txt
LayoutTests/platform/mac-wk2/plugins/refcount-leaks-expected.txt
LayoutTests/plugins/refcount-leaks-expected.txt
LayoutTests/plugins/refcount-leaks.html
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
Source/JavaScriptCore/heap/HeapUtil.h
Source/JavaScriptCore/runtime/JSImmutableButterfly.h

index 0849a5a..6defdd6 100644 (file)
@@ -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  Charlie Turner  <cturner@igalia.com>
 
         [GTK] Unreviewed test gardening
index 369e27c..b40cca5 100644 (file)
@@ -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
index d3e0952..a0ee4fb 100644 (file)
@@ -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();
index e69b2a3..0953d57 100644 (file)
@@ -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();
index dc27d7b..65e68f5 100644 (file)
@@ -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');
index 808d13c..c9edc87 100644 (file)
@@ -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
index 6f752dc..8c6dc35 100644 (file)
@@ -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
index 9e35f69..d51a342 100644 (file)
@@ -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
index 0eb4d79..69f6ee0 100644 (file)
@@ -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>
+
index 0458c56..eec1c39 100644 (file)
@@ -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
index 850b3d2..0c909d0 100644 (file)
@@ -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()) {
index dd6d8de..01cfdbc 100644 (file)
@@ -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
index e5fcc18..3ace0ad 100644 (file)
@@ -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.