Changeset 207061 in webkit


Ignore:
Timestamp:
Oct 11, 2016 12:38:03 AM (8 years ago)
Author:
Carlos Garcia Campos
Message:

Merge r205683 - Heap::isMarked() shouldn't pay the price of concurrent lazy flipping
​https://bugs.webkit.org/show_bug.cgi?id=161760

Reviewed by Mark Lam.
Source/JavaScriptCore:

To fix a race condition in marking, I made Heap::isMarked() and Heap::isLive() atomic by
using flipIfNecessaryConcurrently() instead of flipIfNecessary().

This introduces three unnecessary overheads:

  • isLive() is not called by marking, so that change was not necessary.
  • isMarked() gets calls many times outside of marking, so it shouldn't always do the concurrent thing. This adds isMarkedConcurrently() for use in marking, and reverts isMarked().
  • isMarked() and isMarkedConcurrently() don't actually have to do the lazy flip. They can return false if the flip is necessary.

I added a bunch of debug assertions to make sure that isLive() and isMarked() are not called
during marking.

If we needed to, we could remove most of the calls to isMarkedConcurrently(). As a kind of
optimization, CodeBlock does an initial fixpoint iteration during marking, and so all of the
code called from CodeBlock's fixpoint iterator needs to use isMarkedConcurrently(). But we
could probably arrange for CodeBlock only do fixpoint iterating during the weak reference
thing.

  • bytecode/CodeBlock.cpp:

(JSC::CodeBlock::visitWeakly):
(JSC::CodeBlock::shouldJettisonDueToOldAge):
(JSC::shouldMarkTransition):
(JSC::CodeBlock::propagateTransitions):
(JSC::CodeBlock::determineLiveness):

  • bytecode/PolymorphicAccess.cpp:

(JSC::AccessCase::propagateTransitions):

  • heap/Heap.h:
  • heap/HeapInlines.h:

(JSC::Heap::isLive):
(JSC::Heap::isMarked):
(JSC::Heap::isMarkedConcurrently):

  • heap/MarkedBlock.cpp:

(JSC::MarkedBlock::flipIfNecessarySlow):
(JSC::MarkedBlock::flipIfNecessaryConcurrentlySlow):
(JSC::MarkedBlock::needsFlip):

  • heap/MarkedBlock.h:

(JSC::MarkedBlock::needsFlip):
(JSC::MarkedBlock::flipIfNecessary):
(JSC::MarkedBlock::flipIfNecessaryConcurrently):

  • heap/SlotVisitor.cpp:

(JSC::SlotVisitor::appendToMarkStack):
(JSC::SlotVisitor::markAuxiliary):
(JSC::SlotVisitor::visitChildren):

  • runtime/Structure.cpp:

(JSC::Structure::isCheapDuringGC):
(JSC::Structure::markIfCheap):

Source/WTF:

  • wtf/MainThread.cpp:

(WTF::isMainThreadOrGCThread):
(WTF::mayBeGCThread):

  • wtf/MainThread.h:
Location:
releases/WebKitGTK/webkit-2.14/Source
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/ChangeLog

    r207059 r207061  
     12016-09-08  Filip Pizlo  <fpizlo@apple.com>
     2
     3        Heap::isMarked() shouldn't pay the price of concurrent lazy flipping
     4        https://bugs.webkit.org/show_bug.cgi?id=161760
     5
     6        Reviewed by Mark Lam.
     7       
     8        To fix a race condition in marking, I made Heap::isMarked() and Heap::isLive() atomic by
     9        using flipIfNecessaryConcurrently() instead of flipIfNecessary().
     10       
     11        This introduces three unnecessary overheads:
     12       
     13        - isLive() is not called by marking, so that change was not necessary.
     14       
     15        - isMarked() gets calls many times outside of marking, so it shouldn't always do the
     16          concurrent thing. This adds isMarkedConcurrently() for use in marking, and reverts
     17          isMarked().
     18       
     19        - isMarked() and isMarkedConcurrently() don't actually have to do the lazy flip. They can
     20          return false if the flip is necessary.
     21       
     22        I added a bunch of debug assertions to make sure that isLive() and isMarked() are not called
     23        during marking.
     24       
     25        If we needed to, we could remove most of the calls to isMarkedConcurrently(). As a kind of
     26        optimization, CodeBlock does an initial fixpoint iteration during marking, and so all of the
     27        code called from CodeBlock's fixpoint iterator needs to use isMarkedConcurrently(). But we
     28        could probably arrange for CodeBlock only do fixpoint iterating during the weak reference
     29        thing.
     30
     31        * bytecode/CodeBlock.cpp:
     32        (JSC::CodeBlock::visitWeakly):
     33        (JSC::CodeBlock::shouldJettisonDueToOldAge):
     34        (JSC::shouldMarkTransition):
     35        (JSC::CodeBlock::propagateTransitions):
     36        (JSC::CodeBlock::determineLiveness):
     37        * bytecode/PolymorphicAccess.cpp:
     38        (JSC::AccessCase::propagateTransitions):
     39        * heap/Heap.h:
     40        * heap/HeapInlines.h:
     41        (JSC::Heap::isLive):
     42        (JSC::Heap::isMarked):
     43        (JSC::Heap::isMarkedConcurrently):
     44        * heap/MarkedBlock.cpp:
     45        (JSC::MarkedBlock::flipIfNecessarySlow):
     46        (JSC::MarkedBlock::flipIfNecessaryConcurrentlySlow):
     47        (JSC::MarkedBlock::needsFlip):
     48        * heap/MarkedBlock.h:
     49        (JSC::MarkedBlock::needsFlip):
     50        (JSC::MarkedBlock::flipIfNecessary):
     51        (JSC::MarkedBlock::flipIfNecessaryConcurrently):
     52        * heap/SlotVisitor.cpp:
     53        (JSC::SlotVisitor::appendToMarkStack):
     54        (JSC::SlotVisitor::markAuxiliary):
     55        (JSC::SlotVisitor::visitChildren):
     56        * runtime/Structure.cpp:
     57        (JSC::Structure::isCheapDuringGC):
     58        (JSC::Structure::markIfCheap):
     59
    1602016-09-08  Saam Barati  <sbarati@apple.com>
    261

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/bytecode/CodeBlock.cpp

    r205717 r207061  
    24862486        return;
    24872487
    2488     if (Heap::isMarked(this))
     2488    if (Heap::isMarkedConcurrently(this))
    24892489        return;
    24902490
    … …  
    26292629bool CodeBlock::shouldJettisonDueToOldAge()
    26302630{
    2631     if (Heap::isMarked(this))
     2631    if (Heap::isMarkedConcurrently(this))
    26322632        return false;
    26332633
    … …  
    26442644static bool shouldMarkTransition(DFG::WeakReferenceTransition& transition)
    26452645{
    2646     if (transition.m_codeOrigin && !Heap::isMarked(transition.m_codeOrigin.get()))
     2646    if (transition.m_codeOrigin && !Heap::isMarkedConcurrently(transition.m_codeOrigin.get()))
    26472647        return false;
    26482648   
    2649     if (!Heap::isMarked(transition.m_from.get()))
     2649    if (!Heap::isMarkedConcurrently(transition.m_from.get()))
    26502650        return false;
    26512651   
    … …  
    26782678                Structure* newStructure =
    26792679                    m_vm->heap.structureIDTable().get(newStructureID);
    2680                 if (Heap::isMarked(oldStructure))
     2680                if (Heap::isMarkedConcurrently(oldStructure))
    26812681                    visitor.appendUnbarrieredReadOnlyPointer(newStructure);
    26822682                else
    … …  
    27502750    bool allAreLiveSoFar = true;
    27512751    for (unsigned i = 0; i < dfgCommon->weakReferences.size(); ++i) {
    2752         if (!Heap::isMarked(dfgCommon->weakReferences[i].get())) {
     2752        if (!Heap::isMarkedConcurrently(dfgCommon->weakReferences[i].get())) {
    27532753            allAreLiveSoFar = false;
    27542754            break;
    … …  
    27572757    if (allAreLiveSoFar) {
    27582758        for (unsigned i = 0; i < dfgCommon->weakStructureReferences.size(); ++i) {
    2759             if (!Heap::isMarked(dfgCommon->weakStructureReferences[i].get())) {
     2759            if (!Heap::isMarkedConcurrently(dfgCommon->weakStructureReferences[i].get())) {
    27602760                allAreLiveSoFar = false;
    27612761                break;

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp

    r205717 r207061  
    556556    switch (m_type) {
    557557    case Transition:
    558         if (Heap::isMarked(m_structure->previousID()))
     558        if (Heap::isMarkedConcurrently(m_structure->previousID()))
    559559            visitor.appendUnbarrieredReadOnlyPointer(m_structure.get());
    560560        else

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/heap/Heap.h

    r207055 r207061  
    102102    static bool isLive(const void*);
    103103    static bool isMarked(const void*);
     104    static bool isMarkedConcurrently(const void*);
    104105    static bool testAndSetMarked(HeapVersion, const void*);
    105106    static void setMarked(const void*);

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/heap/HeapInlines.h

    r207055 r207061  
    3636#include <type_traits>
    3737#include <wtf/Assertions.h>
     38#include <wtf/MainThread.h>
    3839#include <wtf/RandomNumber.h>
    3940
    … …  
    7778inline bool Heap::isLive(const void* rawCell)
    7879{
     80    ASSERT(!mayBeGCThread());
    7981    HeapCell* cell = bitwise_cast<HeapCell*>(rawCell);
    8082    if (cell->isLargeAllocation())
    8183        return cell->largeAllocation().isLive();
    8284    MarkedBlock& block = cell->markedBlock();
    83     block.flipIfNecessaryConcurrently(block.vm()->heap.objectSpace().version());
     85    block.flipIfNecessary(block.vm()->heap.objectSpace().version());
    8486    return block.handle().isLiveCell(cell);
    8587}
    … …  
    8789ALWAYS_INLINE bool Heap::isMarked(const void* rawCell)
    8890{
     91    ASSERT(!mayBeGCThread());
    8992    HeapCell* cell = bitwise_cast<HeapCell*>(rawCell);
    9093    if (cell->isLargeAllocation())
    9194        return cell->largeAllocation().isMarked();
    9295    MarkedBlock& block = cell->markedBlock();
    93     block.flipIfNecessaryConcurrently(block.vm()->heap.objectSpace().version());
     96    if (block.needsFlip(block.vm()->heap.objectSpace().version()))
     97        return false;
     98    return block.isMarked(cell);
     99}
     100
     101ALWAYS_INLINE bool Heap::isMarkedConcurrently(const void* rawCell)
     102{
     103    HeapCell* cell = bitwise_cast<HeapCell*>(rawCell);
     104    if (cell->isLargeAllocation())
     105        return cell->largeAllocation().isMarked();
     106    MarkedBlock& block = cell->markedBlock();
     107    if (block.needsFlip(block.vm()->heap.objectSpace().version()))
     108        return false;
     109    WTF::loadLoadFence();
    94110    return block.isMarked(cell);
    95111}

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp

    r202383 r207061  
    6767{
    6868    ASSERT(m_profiler.activeSnapshotBuilder() == this);
    69     ASSERT(Heap::isMarked(cell));
     69    ASSERT(Heap::isMarkedConcurrently(cell));
    7070
    7171    if (hasExistingNodeForCell(cell))

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/heap/MarkedBlock.cpp

    r207055 r207061  
    358358void MarkedBlock::flipIfNecessarySlow()
    359359{
    360     ASSERT(m_version != vm()->heap.objectSpace().version());
     360    ASSERT(needsFlip());
    361361    clearMarks();
    362362}
    … …  
    365365{
    366366    LockHolder locker(m_lock);
    367     if (m_version != vm()->heap.objectSpace().version())
     367    if (needsFlip())
    368368        clearMarks();
    369369}
    … …  
    389389bool MarkedBlock::needsFlip()
    390390{
    391     return vm()->heap.objectSpace().version() != m_version;
     391    return needsFlip(vm()->heap.objectSpace().version());
    392392}
    393393

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/heap/MarkedBlock.h

    r207055 r207061  
    265265    WeakSet& weakSet();
    266266
     267    bool needsFlip(HeapVersion);
    267268    bool needsFlip();
    268269       
    … …  
    463464}
    464465
     466inline bool MarkedBlock::needsFlip(HeapVersion heapVersion)
     467{
     468    return heapVersion != m_version;
     469}
     470
    465471inline void MarkedBlock::flipIfNecessary(HeapVersion heapVersion)
    466472{
    467     if (UNLIKELY(heapVersion != m_version))
     473    if (UNLIKELY(needsFlip(heapVersion)))
    468474        flipIfNecessarySlow();
    469475}
    … …  
    471477inline void MarkedBlock::flipIfNecessaryConcurrently(HeapVersion heapVersion)
    472478{
    473     if (UNLIKELY(heapVersion != m_version))
     479    if (UNLIKELY(needsFlip(heapVersion)))
    474480        flipIfNecessaryConcurrentlySlow();
    475481    WTF::loadLoadFence();

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/heap/SlotVisitor.cpp

    r207055 r207061  
    228228ALWAYS_INLINE void SlotVisitor::appendToMarkStack(ContainerType& container, JSCell* cell)
    229229{
    230     ASSERT(Heap::isMarked(cell));
     230    ASSERT(Heap::isMarkedConcurrently(cell));
    231231    ASSERT(!cell->isZapped());
    232232   
    … …  
    249249   
    250250    if (Heap::testAndSetMarked(m_version, cell)) {
    251         RELEASE_ASSERT(Heap::isMarked(cell));
     251        RELEASE_ASSERT(Heap::isMarkedConcurrently(cell));
    252252        return;
    253253    }
    … …  
    294294ALWAYS_INLINE void SlotVisitor::visitChildren(const JSCell* cell)
    295295{
    296     ASSERT(Heap::isMarked(cell));
     296    ASSERT(Heap::isMarkedConcurrently(cell));
    297297   
    298298    SetCurrentCellScope currentCellScope(*this, cell);

  • releases/WebKitGTK/webkit-2.14/Source/JavaScriptCore/runtime/Structure.cpp

    r204424 r207061  
    11411141    // https://bugs.webkit.org/show_bug.cgi?id=157334
    11421142   
    1143     return (!m_globalObject || Heap::isMarked(m_globalObject.get()))
    1144         && (!storedPrototypeObject() || Heap::isMarked(storedPrototypeObject()));
     1143    return (!m_globalObject || Heap::isMarkedConcurrently(m_globalObject.get()))
     1144        && (!storedPrototypeObject() || Heap::isMarkedConcurrently(storedPrototypeObject()));
    11451145}
    11461146
    … …  
    11481148{
    11491149    if (!isCheapDuringGC())
    1150         return Heap::isMarked(this);
     1150        return Heap::isMarkedConcurrently(this);
    11511151   
    11521152    visitor.appendUnbarrieredReadOnlyPointer(this);

  • releases/WebKitGTK/webkit-2.14/Source/WTF/ChangeLog

    r205730 r207061  
     12016-09-08  Filip Pizlo  <fpizlo@apple.com>
     2
     3        Heap::isMarked() shouldn't pay the price of concurrent lazy flipping
     4        https://bugs.webkit.org/show_bug.cgi?id=161760
     5
     6        Reviewed by Mark Lam.
     7
     8        * wtf/MainThread.cpp:
     9        (WTF::isMainThreadOrGCThread):
     10        (WTF::mayBeGCThread):
     11        * wtf/MainThread.h:
     12
    1132016-09-06  Saam Barati  <sbarati@apple.com>
    214

  • releases/WebKitGTK/webkit-2.14/Source/WTF/wtf/MainThread.cpp

    r202736 r207061  
    11/*
    2  * Copyright (C) 2007, 2008, 2015 Apple Inc. All rights reserved.
     2 * Copyright (C) 2007, 2008, 2015-2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    211211bool isMainThreadOrGCThread()
    212212{
    213     if (isGCThread->isSet() && **isGCThread)
     213    if (mayBeGCThread())
    214214        return true;
    215215
    … …  
    217217}
    218218
     219bool mayBeGCThread()
     220{
     221    return isGCThread->isSet() && **isGCThread;
     222}
     223
    219224} // namespace WTF

  • releases/WebKitGTK/webkit-2.14/Source/WTF/wtf/MainThread.h

    r202439 r207061  
    11/*
    2  * Copyright (C) 2007, 2008, 2010 Apple Inc. All rights reserved.
     2 * Copyright (C) 2007, 2008, 2010, 2016 Apple Inc. All rights reserved.
    33 * Copyright (C) 2007 Justin Haygood (jhaygood@reaktix.com)
    44 *
    … …  
    6969
    7070WTF_EXPORT_PRIVATE void registerGCThread();
     71WTF_EXPORT_PRIVATE bool mayBeGCThread();
    7172WTF_EXPORT_PRIVATE bool isMainThreadOrGCThread();
    7273
    … …  
    8990
    9091using WTF::callOnMainThread;
     92using WTF::canAccessThreadLocalDataForThread;
     93using WTF::isMainThread;
     94using WTF::isMainThreadOrGCThread;
     95using WTF::isUIThread;
     96using WTF::isWebThread;
     97using WTF::mayBeGCThread;
     98using WTF::setMainThreadCallbacksPaused;
    9199#if PLATFORM(COCOA)
    92100using WTF::callOnWebThreadOrDispatchAsyncOnMainThread;
    93101#endif
    94 using WTF::setMainThreadCallbacksPaused;
    95 using WTF::isMainThread;
    96 using WTF::isMainThreadOrGCThread;
    97 using WTF::canAccessThreadLocalDataForThread;
    98 using WTF::isUIThread;
    99 using WTF::isWebThread;
    100102#if USE(WEB_THREAD)
    101103using WTF::initializeWebThread;
Note: See TracChangeset for help on using the changeset viewer.