[Webkit-unassigned] [Bug 64493] GC allocation fast path has too many operations

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64493


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #100739|1                           |0
        is obsolete|                            |




--- Comment #8 from Oliver Hunt <oliver at apple.com>  2011-07-13 18:24:41 PST ---
(From update of attachment 100739)
View in context: https://bugs.webkit.org/attachment.cgi?id=100739&action=review

r- due to the comments above, but also because the exports changes will no doubt break the $%#& windows build.

> Source/JavaScriptCore/heap/Heap.h:264
> +        clearAllocCache();

These functions are used for things other than GC -- clearAllocCache has the effect of clearing mark bits so this seems like it could cause badness, what am i missing?

> Source/JavaScriptCore/heap/Heap.h:279
> +        clearAllocCache();

ditto

> Source/JavaScriptCore/heap/MarkedBlock.h:101
> +        // These should be called immediately after a block is created.
> +        // Blessing for fast path creates a linked list, while blessing for
> +        // slow path creates dummy cells.
> +        FreeCell* blessNewBlockForFastPath();
> +        void blessNewBlockForSlowPath();

I think this is most easily guarantee by making the MarkBlock constructor handle that itself, add an enum along the lines of
enum MarkBlockAllocationPath { AllocationPathSlow, AllocationPathFast };
make the MarkBlock constructor take a MarkBlockAllocationPath parameter in its constructor, and decide which version of bless should be used in the constructor.

> Source/JavaScriptCore/heap/MarkedBlock.h:105
> +        void clearAllocCache(FreeCell* firstFree);

I don't like this name, i find it confusing -- it says it is clearing a cache, when in reality it's also clearing all the mark bits as we would for a GC pass

> Source/JavaScriptCore/heap/NewSpace.h:159
> +        MarkedBlock::FreeCell* firstFree = sizeClass.firstFree;
> +        if (!firstFree) {
> +            // There are two possibilities for why we got here:
> +            // 1) We've exhausted the allocation cache for curBlock, in which case
> +            //    curBlock == nextBlock, and we know that there is no reason to
> +            //    repeat a lazy sweep of nextBlock because we won't find anything.
> +            // 2) Allocation caches have been cleared, in which case nextBlock may
> +            //    have (and most likely does have) free cells, so we almost certainly
> +            //    should do a lazySweep for nextBlock. This also implies that
> +            //    curBlock == 0.
> +            
> +            if (sizeClass.curBlock) {
> +                ASSERT(sizeClass.curBlock == sizeClass.nextBlock);
> +                m_waterMark += sizeClass.nextBlock->capacity();
> +                sizeClass.nextBlock = sizeClass.nextBlock->next();
> +                sizeClass.curBlock = 0;
> +            }
> +            
> +            for (MarkedBlock*& block = sizeClass.nextBlock ; block; block = block->next()) {
> +                firstFree = block->lazySweep();
> +                if (firstFree) {
> +                    sizeClass.firstFree = firstFree;
> +                    sizeClass.curBlock = block;
> +                    break;
> +                }
> +                
> +                m_waterMark += block->capacity();
> +            }
> +            
> +            if (!firstFree)
> +                return 0;
>          }
> -
> -        return 0;
> +        
> +        ASSERT(firstFree);
> +        
> +        sizeClass.firstFree = firstFree->next;
> +        return static_cast<void*>(firstFree);

This would be nicer as an early return:
if (MarkedBlock::FreeCell* firstFree = sizeClass.firstFree) {
     sizeClass.firstFree = firstFree->next;
     return firstFree;
}
....

I think that makes the fast path much clearer and more obvious

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.