[Webkit-unassigned] [Bug 64493] GC allocation fast path has too many operations
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 13 19:18:38 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=64493
--- Comment #11 from Filip Pizlo <fpizlo at apple.com> 2011-07-13 19:18:38 PST ---
(In reply to comment #8)
> (From update of attachment 100739 [details])
> 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?
It's almost always safe to call this function, in the sense that it just undoes free-lists in SizeClasses that had them. It brings the heap back into a canonical form, where each MarkedBlock consists of a bunch of fully initialized JSCells, with mark bits indicating whether these JSCells are actually live.
In particular, this method only clears mark bits for those objects that are on the free list. An object is placed on the free list only if it is not marked, and as it's placed, it also has its mark bit set to ensure that the allocator fast path doesn't have to do it. That means that every object still on a free list has the mark bit set, and should have its mark bit cleared if the free list is destroyed.
Another implication of placing objects on the free list is that they no longer have a valid JSCell in them, so anyone iterating over JSCells will get massively confused. Hence the need to call clearAllocCache() before doing most anything that iterates over all cells or all blocks - this brings the block back into a "canonical" state where (1) mark bits are only set for things that are actually allocated and (2) all cells contains a valid JSCell.
This brings up a possible idea for a better name for this method: canonicalizeBlocks()?
>
> > 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.
The reason why I didn't do it this way is that blessNewBlockForFastPath() returns a FreeCell*, which is not stored anywhere in MarkedBlock. It's only stored in SizeClass, because it's only relevant for the block that is currently used for allocation.
I'm happy to change the code to do construction in one go, and have MarkedBlock::create() also return a FreeCell* (via a reference parameter) and then thread that through the relevant methods in Heap and NewSpace.
> > 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
I made it a late return because this covers both the I-already-have-a-free-cell case and the I-need-to-scavenge-for-a-free-cell case. This way the NewSpace::allocate() method has two clear phases: phase one ensures that it's possible to allocate in the given SizeClass, and the second phase (the "fast path") performs the allocation.
And anyway, the real fast path is in Heap::allocate(). NewSpace::allocate() is typically called with SizeClass.firstFree == 0. The only exceptions are if NewSpace::allocate() returns 0, and the Heap::allocate() decides to deal with it by allocating a new block, and then calls NewSpace::allocate() again.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list