Changeset 242912 in webkit

Timestamp:

Mar 13, 2019 3:05:19 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

Remove unneeded --tradeDestructorBlocks option.
​https://bugs.webkit.org/show_bug.cgi?id=195698
<rdar://problem/39681388>

Reviewed by Yusuke Suzuki.

There's no reason why we would ever want --tradeDestructorBlocks to be false.

Also, there was an assertion in BlockDirectory::endMarking() for when
(!Options::tradeDestructorBlocks() && needsDestruction()). This assertion is
outdated because the BlockDirectory's m_empty set used to mean the set of all
blocks that have no live (as in not reachable by GC) objects and dead objects
also do not require destructors to be called on them. The current meaning of
m_empty is that it is the set of all blocks that have no live objects,
independent of whether they needs destructors to be called on them or not.
The assertion is no longer valid for the new meaning of m_empty as m_empty may
now contain destructible blocks. This assertion is now removed as part of this
patch.

• heap/BlockDirectory.cpp:

(JSC::BlockDirectory::endMarking):

• heap/LocalAllocator.cpp:

(JSC::LocalAllocator::tryAllocateWithoutCollecting):

• runtime/Options.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/BlockDirectory.cpp (modified) (2 diffs)
• heap/LocalAllocator.cpp (modified) (2 diffs)
• runtime/Options.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-03-13  Mark Lam  <mark.lam@apple.com>
+
+        Remove unneeded --tradeDestructorBlocks option.
+        https://bugs.webkit.org/show_bug.cgi?id=195698
+        <rdar://problem/39681388>
+
+        Reviewed by Yusuke Suzuki.
+
+        There's no reason why we would ever want --tradeDestructorBlocks to be false.
+
+        Also, there was an assertion in BlockDirectory::endMarking() for when
+        (!Options::tradeDestructorBlocks() && needsDestruction()).  This assertion is
+        outdated because the BlockDirectory's m_empty set used to mean the set of all
+        blocks that have no live (as in not reachable by GC) objects and dead objects
+        also do not require destructors to be called on them.  The current meaning of
+        m_empty is that it is the set of all blocks that have no live objects,
+        independent of whether they needs destructors to be called on them or not.
+        The assertion is no longer valid for the new meaning of m_empty as m_empty may
+        now contain destructible blocks.  This assertion is now removed as part of this
+        patch.
+
+        * heap/BlockDirectory.cpp:
+        (JSC::BlockDirectory::endMarking):
+        * heap/LocalAllocator.cpp:
+        (JSC::LocalAllocator::tryAllocateWithoutCollecting):
+        * runtime/Options.h:
+
 2019-03-13  Dominik Infuehr  <dinfuehr@igalia.com>
 

trunk/Source/JavaScriptCore/heap/BlockDirectory.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2012-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -250 +250 @@
     // vectors.
     
-    if (!Options::tradeDestructorBlocks() && needsDestruction()) {
-        ASSERT(m_empty.isEmpty());
-        m_canAllocateButNotEmpty = m_live & ~m_markingRetired;
-    } else {
-        m_empty = m_live & ~m_markingNotEmpty;
-        m_canAllocateButNotEmpty = m_live & m_markingNotEmpty & ~m_markingRetired;
-    }
-    
+    m_empty = m_live & ~m_markingNotEmpty;
+    m_canAllocateButNotEmpty = m_live & m_markingNotEmpty & ~m_markingRetired;
+
     if (needsDestruction()) {
         // There are some blocks that we didn't allocate out of in the last cycle, but we swept them. This

trunk/Source/JavaScriptCore/heap/LocalAllocator.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2018-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -185 +185 @@
     }
     
-    if (Options::stealEmptyBlocksFromOtherAllocators()
-        && (Options::tradeDestructorBlocks() || !m_directory->needsDestruction())) {
+    if (Options::stealEmptyBlocksFromOtherAllocators()) {
         if (MarkedBlock::Handle* block = m_directory->m_subspace->findEmptyBlockToSteal()) {
             RELEASE_ASSERT(block->alignedMemoryAllocator() == m_directory->m_subspace->alignedMemoryAllocator());

trunk/Source/JavaScriptCore/runtime/Options.h

@@ -1 +1 @@
 /*
- * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -253 +253 @@
     v(bool, useBumpAllocator, true, Normal, nullptr) \
     v(bool, stealEmptyBlocksFromOtherAllocators, true, Normal, nullptr) \
-    v(bool, tradeDestructorBlocks, true, Normal, nullptr) \
     v(bool, eagerlyUpdateTopCallFrame, false, Normal, nullptr) \
     \