Bug 126555

Created attachment 220912 [details] Patch

Comment on attachment 220912 [details] Patch Attachment 220912 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/4947627439292416

Comment on attachment 220912 [details] Patch Attachment 220912 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/5079382943072256

Comment on attachment 220912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220912&action=review r=me, with some suggestions I don't understand the EFL bot's objection, so I think we should ignore it. > Source/JavaScriptCore/ChangeLog:12 > + and a new generation of CopiedBlocks. During each mutator cycle new CopiedSpace allocations reside I think you meant "set of CopiedBlocks". > Source/JavaScriptCore/ChangeLog:18 > + One key thing to remember is that both new and old generation objects in the MarkedSpace can > + refer to old or new generation allocations in CopiedSpace. In other words, the generations in CopiedSpace > + are independent of those in the MarkedSpace. This is why we must fire write barriers when assigning > + to an old (MarkedSpace) object's Butterfly. I don't think it's right to say that the spaces are "independent". Both spaces promote at the same time. That makes them dependent. I think I would just remove that middle sentence. The rest is good. > Source/JavaScriptCore/heap/CopiedBlockInlines.h:42 > + // this if this block was allocated during the last cycle. Remove extra "this". > Source/JavaScriptCore/heap/CopiedBlockInlines.h:46 > + // We want to add to live bytes if the owner isn't part of the remembered set or > + // this if this block was allocated during the last cycle. > + // If we always added live bytes we would double count for elements in the remembered > + // set across collections. > + // If we didn't always add live bytes to new blocks, we'd get too few. > + if (!isOwnerRemembered || !m_isOld) { Let's factor this into a helper function -- maybe "shouldReportLiveBytes" -- and call the helper function at the call site, and assert that it's true inside this function. That way, you can remove the double checking inside the function, and establish consistency between what SlotVisitor records and what CopiedBlock records. I think you could tighten this comment by starting with each case we want to avoid: "Don't report when copying on behalf of a remembered object because..."; "Don't report when copying an old object because..." > Source/JavaScriptCore/heap/CopiedSpace.cpp:238 > + DoublyLinkedList<CopiedBlock>* toSpace, *fromSpace; Two lines, please. Multiple pointer declaration is just too easy to get wrong. > Source/JavaScriptCore/heap/CopiedSpaceInlines.h:97 > +template <HeapOperation collectionType> > inline void CopiedSpace::recycleEvacuatedBlock(CopiedBlock* block) I think it would be better just to pass HeapOperation as an argument. That way, you could remove the if at the call site. Since this function is inlined, no efficiency will be lost. > Source/JavaScriptCore/heap/CopiedSpaceInlines.h:185 > + DoublyLinkedList<CopiedBlock>* toSpace, *fromSpace, *oversizeBlocks; Multiple lines, please. > Source/JavaScriptCore/heap/CopiedSpaceInlines.h:217 > + // TODO: Make sure we accurately keep track of the total live bytes for EdenCollections. Is this still a TODO? It seems like you did it. > Source/JavaScriptCore/heap/CopiedSpaceInlines.h:247 > + double totalFragmentation = ((double)totalLiveBytes + markedSpaceBytes) / ((double)totalUsableBytes + markedSpaceBytes); static_cast, please. > Source/JavaScriptCore/heap/GCThreadSharedData.cpp:206 > + unsigned index = 0; > + CopiedBlock* block = m_copiedSpace->m_newGen.fromSpace->head(); > + // First we fill up the pre-existing space in the vector. > + while (block) { > + if (index >= m_blocksToCopy.size()) > + break; > + m_blocksToCopy[index++] = block; > + block = block->next(); > + } > + > + // If we have no more blocks in the linked list we reduce the size of the vector > + // to the size of the linked list. We do the resize here instead of at the very > + // beginning because it's O(n) to get the length of the linked list of blocks. > + if (!block) > + m_blocksToCopy.resize(index); > + > + // If we exhausted the pre-existing space in the vector and have more blocks then start > + // appending to the end of the vector. > + while (block) { > + m_blocksToCopy.append(block); > + block = block->next(); > + } You can just do a normal shrink(0) followed by append for each item. The backing store will not deallocate from a shrink(0) -- only from clear(), shrinkToFit(), or shrinkCapacity(). > Source/JavaScriptCore/heap/SlotVisitorInlines.h:231 > + bool isRemembered = heap()->operationInProgress() == EdenCollection && MarkedBlock::blockFor(owner)->isRemembered(owner); > + if (!isRemembered) > + m_bytesCopied += bytes; This check is different from the check done inside CopiedBlock. See my comment there for a suggestion of how to improve this.

Created attachment 221167 [details] Patch

(In reply to comment #5) > Created an attachment (id=221167) [details] > Patch Just to make sure we're on the same page w.r.t. SlotVisitor::copyLater, CopiedBlock::shouldReportLiveBytes, and CopiedBlock::reportLiveBytes.

Comment on attachment 221167 [details] Patch Attachment 221167 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/6525023300878336

Comment on attachment 221167 [details] Patch Attachment 221167 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/6152784122478592

Comment on attachment 221167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221167&action=review r=me You should send email to webkit-dev inviting the EFL and GTK maintainers to resolved their respective build issues. > Source/WTF/wtf/Platform.h:760 > +#define ENABLE_GGC 1 -1 point for Gryffindor!

Created attachment 221189 [details] Patch

(In reply to comment #10) > Created an attachment (id=221189) [details] > Patch One more upload for EWS to chew on.

Committed r162017: <http://trac.webkit.org/changeset/162017>

Comment on attachment 221189 [details] Patch Cleared review? from attachment 221189 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).