Changeset 190310 in webkit


Ignore:
Timestamp:
Sep 29, 2015, 9:39:24 AM (10 years ago)
Author:
fpizlo@apple.com
Message:

GC copy phase spans too many files
​https://bugs.webkit.org/show_bug.cgi?id=149586

Reviewed by Andreas Kling.

Source/JavaScriptCore:

This puts the core logic of the copy phase into Heap::copyBackingStores(). Now, instead of
using many helpers in many places, the actual algorithm is all in one place.

This lets me do a lot of simplification.

  • CopyVisitor no longer requires that you call startCopying() before, and doneCopying() and WTF::releaseFastMallocFreeMemoryForThisThread() after. The constructor and destructor now do this for you.
  • CopyVisitor no longer contains the algorithm that drives copying. That's all in Heap::copyBackingStores() now. Basically, copyBackingStores() glues together the new WTF::ParallelVectorIterator with the copying algorithm that we used to have in CopyVisitor::copyFromShared().
  • Lots of stuff that was in headers is now in .cpp files. That includes all non-hot-path code in CopyVisitor. Also, the code for copying in HeapInlines.h is now in ParallelVectorVisotor, and it's only included by Heap.cpp.

Overall, I like this direction for the GC. I don't think it's useful for Heap.cpp to have
calls to algorithms in some other file, unless those algorithms are either reusable or just
very dense. That's not actually true for the copy phase, and it's probably not true for
some other stuff like marking. I'll probably do the same refactoring for marking in another
bug.

This should have no effect on performance.

  • heap/CopyVisitor.cpp:

(JSC::CopyVisitor::CopyVisitor):
(JSC::CopyVisitor::~CopyVisitor):
(JSC::CopyVisitor::copyFromShared): Deleted.

  • heap/CopyVisitor.h:
  • heap/CopyVisitorInlines.h:

(JSC::CopyVisitor::checkIfShouldCopy):
(JSC::CopyVisitor::allocateNewSpaceSlow):
(JSC::CopyVisitor::didCopy):
(JSC::CopyVisitor::visitItem): Deleted.
(JSC::CopyVisitor::startCopying): Deleted.
(JSC::CopyVisitor::doneCopying): Deleted.

  • heap/Heap.cpp:

(JSC::Heap::copyBackingStores):

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

(JSC::Heap::unregisterWeakGCMap):
(JSC::Heap::getNextBlocksToCopy): Deleted.

Source/WTF:

Extract the load balancing algorithm used by the GC's copy phase into a reusable template.
The GC copy phase now uses this.

  • WTF.vcxproj/WTF.vcxproj:
  • WTF.vcxproj/WTF.vcxproj.filters:
  • WTF.xcodeproj/project.pbxproj:
  • wtf/CMakeLists.txt:
  • wtf/ParallelVectorIterator.h: Added.

(WTF::ParallelVectorIterator::ParallelVectorIterator):
(WTF::ParallelVectorIterator::iterate):

Location:
trunk/Source
Files:
1 added
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r190305 r190310  
     12015-09-29  Filip Pizlo  <fpizlo@apple.com>
     2
     3        GC copy phase spans too many files
     4        https://bugs.webkit.org/show_bug.cgi?id=149586
     5
     6        Reviewed by Andreas Kling.
     7
     8        This puts the core logic of the copy phase into Heap::copyBackingStores(). Now, instead of
     9        using many helpers in many places, the actual algorithm is all in one place.
     10
     11        This lets me do a lot of simplification.
     12
     13        - CopyVisitor no longer requires that you call startCopying() before, and doneCopying() and
     14          WTF::releaseFastMallocFreeMemoryForThisThread() after. The constructor and destructor now
     15          do this for you.
     16
     17        - CopyVisitor no longer contains the algorithm that drives copying. That's all in
     18          Heap::copyBackingStores() now. Basically, copyBackingStores() glues together the new
     19          WTF::ParallelVectorIterator with the copying algorithm that we used to have in
     20          CopyVisitor::copyFromShared().
     21
     22        - Lots of stuff that was in headers is now in .cpp files. That includes all non-hot-path
     23          code in CopyVisitor. Also, the code for copying in HeapInlines.h is now in
     24          ParallelVectorVisotor, and it's only included by Heap.cpp.
     25
     26        Overall, I like this direction for the GC. I don't think it's useful for Heap.cpp to have
     27        calls to algorithms in some other file, unless those algorithms are either reusable or just
     28        very dense. That's not actually true for the copy phase, and it's probably not true for
     29        some other stuff like marking. I'll probably do the same refactoring for marking in another
     30        bug.
     31
     32        This should have no effect on performance.
     33
     34        * heap/CopyVisitor.cpp:
     35        (JSC::CopyVisitor::CopyVisitor):
     36        (JSC::CopyVisitor::~CopyVisitor):
     37        (JSC::CopyVisitor::copyFromShared): Deleted.
     38        * heap/CopyVisitor.h:
     39        * heap/CopyVisitorInlines.h:
     40        (JSC::CopyVisitor::checkIfShouldCopy):
     41        (JSC::CopyVisitor::allocateNewSpaceSlow):
     42        (JSC::CopyVisitor::didCopy):
     43        (JSC::CopyVisitor::visitItem): Deleted.
     44        (JSC::CopyVisitor::startCopying): Deleted.
     45        (JSC::CopyVisitor::doneCopying): Deleted.
     46        * heap/Heap.cpp:
     47        (JSC::Heap::copyBackingStores):
     48        * heap/Heap.h:
     49        * heap/HeapInlines.h:
     50        (JSC::Heap::unregisterWeakGCMap):
     51        (JSC::Heap::getNextBlocksToCopy): Deleted.
     52
    1532015-09-29  Youenn Fablet  <youenn.fablet@crf.canon.fr>
    254

  • trunk/Source/JavaScriptCore/heap/CopyVisitor.cpp

    r190151 r190310  
    11/*
    2  * Copyright (C) 2012 Apple Inc. All rights reserved.
     2 * Copyright (C) 2012, 2015 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4040    : m_heap(heap)
    4141{
     42    ASSERT(!m_copiedAllocator.isValid());
     43    CopiedBlock* block = nullptr;
     44    m_heap.m_storageSpace.doneFillingBlock(nullptr, &block);
     45    m_copiedAllocator.setCurrentBlock(block);
    4246}
    4347
    44 void CopyVisitor::copyFromShared()
     48CopyVisitor::~CopyVisitor()
    4549{
    46     size_t next, end;
    47     m_heap.getNextBlocksToCopy(next, end);
    48     while (next < end) {
    49         for (; next < end; ++next) {
    50             CopiedBlock* block = m_heap.m_blocksToCopy[next];
    51             if (!block->hasWorkList())
    52                 continue;
    53 
    54             CopyWorkList& workList = block->workList();
    55             for (CopyWorkList::iterator it = workList.begin(); it != workList.end(); ++it)
    56                 visitItem(*it);
    57 
    58             ASSERT(!block->liveBytes());
    59             m_heap.m_storageSpace.recycleEvacuatedBlock(block, m_heap.operationInProgress());
    60         }
    61         m_heap.getNextBlocksToCopy(next, end);
    62     }
    63     ASSERT(next == end);
     50    if (m_copiedAllocator.isValid())
     51        m_heap.m_storageSpace.doneFillingBlock(m_copiedAllocator.resetCurrentBlock(), nullptr);
     52   
     53    WTF::releaseFastMallocFreeMemoryForThisThread();
    6454}
    6555

  • trunk/Source/JavaScriptCore/heap/CopyVisitor.h

    r190151 r190310  
    11/*
    2  * Copyright (C) 2012 Apple Inc. All rights reserved.
     2 * Copyright (C) 2012, 2015 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    2828
    2929#include "CopiedSpace.h"
     30#include <wtf/Noncopyable.h>
    3031
    3132namespace JSC {
    … …  
    3536
    3637class CopyVisitor {
     38    WTF_MAKE_NONCOPYABLE(CopyVisitor);
    3739public:
    3840    CopyVisitor(Heap&);
     41    ~CopyVisitor();
    3942
    4043    void copyFromShared();
    41 
    42     void startCopying();
    43     void doneCopying();
    4444
    4545    // Low-level API for copying, appropriate for cases where the object's heap references

  • trunk/Source/JavaScriptCore/heap/CopyVisitorInlines.h

    r190151 r190310  
    3535namespace JSC {
    3636
    37 inline void CopyVisitor::visitItem(CopyWorklistItem item)
    38 {
    39     if (item.token() == ButterflyCopyToken) {
    40         JSObject::copyBackingStore(item.cell(), *this, ButterflyCopyToken);
    41         return;
    42     }
    43    
    44     item.cell()->methodTable()->copyBackingStore(item.cell(), *this, item.token());
    45 }
    46 
    4737inline bool CopyVisitor::checkIfShouldCopy(void* oldPtr)
    4838{
    … …  
    7666}
    7767
    78 inline void CopyVisitor::startCopying()
    79 {
    80     ASSERT(!m_copiedAllocator.isValid());
    81     CopiedBlock* block = 0;
    82     m_heap.m_storageSpace.doneFillingBlock(m_copiedAllocator.resetCurrentBlock(), &block);
    83     m_copiedAllocator.setCurrentBlock(block);
    84 }
    85 
    86 inline void CopyVisitor::doneCopying()
    87 {
    88     if (!m_copiedAllocator.isValid())
    89         return;
    90 
    91     m_heap.m_storageSpace.doneFillingBlock(m_copiedAllocator.resetCurrentBlock(), 0);
    92 }
    93 
    9468inline void CopyVisitor::didCopy(void* ptr, size_t bytes)
    9569{

  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r190269 r190310  
    5454#include <wtf/RAMSize.h>
    5555#include <wtf/CurrentTime.h>
     56#include <wtf/ParallelVectorIterator.h>
    5657#include <wtf/ProcessID.h>
    5758
    … …  
    6162namespace JSC {
    6263
    63 namespace { 
     64namespace {
    6465
    6566static const size_t largeHeapSize = 32 * MB; // About 1.5X the average webpage.
    … …  
    620621
    621622    if (m_storageSpace.shouldDoCopyPhase()) {
    622         {
    623             LockHolder locker(m_copyLock);
    624             if (m_operationInProgress == EdenCollection) {
    625                 // Reset the vector to be empty, but don't throw away the backing store.
    626                 m_blocksToCopy.shrink(0);
    627                 for (CopiedBlock* block = m_storageSpace.m_newGen.fromSpace->head(); block; block = block->next())
    628                     m_blocksToCopy.append(block);
    629             } else {
    630                 ASSERT(m_operationInProgress == FullCollection);
    631                 WTF::copyToVector(m_storageSpace.m_blockSet, m_blocksToCopy);
    632             }
    633             m_copyIndex = 0;
     623        if (m_operationInProgress == EdenCollection) {
     624            // Reset the vector to be empty, but don't throw away the backing store.
     625            m_blocksToCopy.shrink(0);
     626            for (CopiedBlock* block = m_storageSpace.m_newGen.fromSpace->head(); block; block = block->next())
     627                m_blocksToCopy.append(block);
     628        } else {
     629            ASSERT(m_operationInProgress == FullCollection);
     630            WTF::copyToVector(m_storageSpace.m_blockSet, m_blocksToCopy);
    634631        }
    635632
     633        ParallelVectorIterator<Vector<CopiedBlock*>> iterator(
     634            m_blocksToCopy, s_blockFragmentLength);
     635
     636        // Note that it's safe to use the [&] capture list here, even though we're creating a task
     637        // that other threads run. That's because after runFunctionInParallel() returns, the task
     638        // we have created is not going to be running anymore. Hence, everything on the stack here
     639        // outlives the task.
    636640        m_helperClient.runFunctionInParallel(
    637             [this] () {
     641            [&] () {
    638642                CopyVisitor copyVisitor(*this);
    639                 copyVisitor.startCopying();
    640                 copyVisitor.copyFromShared();
    641                 copyVisitor.doneCopying();
    642                 WTF::releaseFastMallocFreeMemoryForThisThread();
     643               
     644                iterator.iterate(
     645                    [&] (CopiedBlock* block) {
     646                        if (!block->hasWorkList())
     647                            return;
     648                       
     649                        CopyWorkList& workList = block->workList();
     650                        for (CopyWorklistItem item : workList) {
     651                            if (item.token() == ButterflyCopyToken) {
     652                                JSObject::copyBackingStore(
     653                                    item.cell(), copyVisitor, ButterflyCopyToken);
     654                                continue;
     655                            }
     656                           
     657                            item.cell()->methodTable()->copyBackingStore(
     658                                item.cell(), copyVisitor, item.token());
     659                        }
     660                       
     661                        ASSERT(!block->liveBytes());
     662                        m_storageSpace.recycleEvacuatedBlock(block, m_operationInProgress);
     663                    });
    643664            });
    644665    }

  • trunk/Source/JavaScriptCore/heap/Heap.h

    r190267 r190310  
    347347    size_t threadDupStrings();
    348348
    349     void getNextBlocksToCopy(size_t&, size_t&);
    350 
    351349    const HeapType m_heapType;
    352350    const size_t m_ramSize;
    … …  
    438436    HashSet<void*> m_opaqueRoots;
    439437
    440     Lock m_copyLock;
    441438    Vector<CopiedBlock*> m_blocksToCopy;
    442     size_t m_copyIndex { 0 };
    443439    static const size_t s_blockFragmentLength = 32;
    444440

  • trunk/Source/JavaScriptCore/heap/HeapInlines.h

    r190151 r190310  
    327327}
    328328   
    329 inline void Heap::getNextBlocksToCopy(size_t& start, size_t& end)
    330 {
    331     LockHolder locker(m_copyLock);
    332     start = m_copyIndex;
    333     end = std::min(m_blocksToCopy.size(), m_copyIndex + s_blockFragmentLength);
    334     m_copyIndex = end;
    335 }
    336 
    337329} // namespace JSC
    338330

  • trunk/Source/WTF/ChangeLog

    r190268 r190310  
     12015-09-29  Filip Pizlo  <fpizlo@apple.com>
     2
     3        GC copy phase spans too many files
     4        https://bugs.webkit.org/show_bug.cgi?id=149586
     5
     6        Reviewed by Andreas Kling.
     7
     8        Extract the load balancing algorithm used by the GC's copy phase into a reusable template.
     9        The GC copy phase now uses this.
     10
     11        * WTF.vcxproj/WTF.vcxproj:
     12        * WTF.vcxproj/WTF.vcxproj.filters:
     13        * WTF.xcodeproj/project.pbxproj:
     14        * wtf/CMakeLists.txt:
     15        * wtf/ParallelVectorIterator.h: Added.
     16        (WTF::ParallelVectorIterator::ParallelVectorIterator):
     17        (WTF::ParallelVectorIterator::iterate):
     18
    1192015-09-26  Filip Pizlo  <fpizlo@apple.com>
    220

  • trunk/Source/WTF/WTF.vcxproj/WTF.vcxproj

    r190267 r190310  
    253253    <ClInclude Include="..\wtf\ParallelJobsLibdispatch.h" />
    254254    <ClInclude Include="..\wtf\ParallelJobsOpenMP.h" />
     255    <ClInclude Include="..\wtf\ParallelVectorIterator.h" />
    255256    <ClInclude Include="..\wtf\ParkingLot.h" />
    256257    <ClInclude Include="..\wtf\PassRefPtr.h" />

  • trunk/Source/WTF/WTF.vcxproj/WTF.vcxproj.filters

    r190267 r190310  
    564564    </ClInclude>
    565565    <ClInclude Include="..\wtf\ParallelJobsOpenMP.h">
     566      <Filter>wtf</Filter>
     567    </ClInclude>
     568    <ClInclude Include="..\wtf\ParallelVectorIterator.h">
    566569      <Filter>wtf</Filter>
    567570    </ClInclude>

  • trunk/Source/WTF/WTF.xcodeproj/project.pbxproj

    r190267 r190310  
    5050                0FE4479D1B7AAA03009498EB /* WordLock.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FE4479B1B7AAA03009498EB /* WordLock.h */; };
    5151                0FEB3DCF1BB5D684009D7AAD /* SharedTask.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FEB3DCE1BB5D684009D7AAD /* SharedTask.h */; };
     52                0FEB3DD11BB7366B009D7AAD /* ParallelVectorIterator.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FEB3DD01BB7366B009D7AAD /* ParallelVectorIterator.h */; };
    5253                0FED67B61B22D4D80066CE15 /* TinyPtrSet.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FED67B51B22D4D80066CE15 /* TinyPtrSet.h */; };
    5354                0FFF19DC1BB334EB00886D91 /* ParallelHelperPool.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 0FFF19DA1BB334EB00886D91 /* ParallelHelperPool.cpp */; };
    … …  
    342343                0FE4479B1B7AAA03009498EB /* WordLock.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WordLock.h; sourceTree = "<group>"; };
    343344                0FEB3DCE1BB5D684009D7AAD /* SharedTask.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SharedTask.h; sourceTree = "<group>"; };
     345                0FEB3DD01BB7366B009D7AAD /* ParallelVectorIterator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ParallelVectorIterator.h; sourceTree = "<group>"; };
    344346                0FED67B51B22D4D80066CE15 /* TinyPtrSet.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = TinyPtrSet.h; sourceTree = "<group>"; };
    345347                0FFF19DA1BB334EB00886D91 /* ParallelHelperPool.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ParallelHelperPool.cpp; sourceTree = "<group>"; };
    … …  
    831833                                A8A472E6151A825B004123FF /* ParallelJobs.h */,
    832834                                A8A472E9151A825B004123FF /* ParallelJobsLibdispatch.h */,
     835                                0FEB3DD01BB7366B009D7AAD /* ParallelVectorIterator.h */,
    833836                                0F824A641B7443A0002E345D /* ParkingLot.cpp */,
    834837                                0F824A651B7443A0002E345D /* ParkingLot.h */,
    … …  
    10861089                                0FB14E1B1810E1DC009B6B4D /* BagToHashMap.h in Headers */,
    10871090                                8134013915B092FD001FF0B8 /* Base64.h in Headers */,
     1091                                0FEB3DD11BB7366B009D7AAD /* ParallelVectorIterator.h in Headers */,
    10881092                                A8A473A9151A825B004123FF /* bignum-dtoa.h in Headers */,
    10891093                                A8A473AB151A825B004123FF /* bignum.h in Headers */,

  • trunk/Source/WTF/wtf/CMakeLists.txt

    r190267 r190310  
    6161    PageBlock.h
    6262    PageReservation.h
     63    ParallelHelperPool.h
    6364    ParallelJobs.h
    6465    ParallelJobsGeneric.h
    6566    ParallelJobsLibdispatch.h
    6667    ParallelJobsOpenMP.h
     68    ParallelVectorIterator.h
    6769    ParkingLot.h
    6870    PassRefPtr.h
Note: See TracChangeset for help on using the changeset viewer.