[webkit-reviews] review granted: [Bug 25246] Jumps may fail to be linked correctly on x86_64. : [Attachment 29551] The patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 25246: Jumps may fail to be linked correctly on x86_64.
https://bugs.webkit.org/show_bug.cgi?id=25246

Attachment 29551: The patch
https://bugs.webkit.org/attachment.cgi?id=29551&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
> +    // All entries of the same size share a single entry
> +    // in the AVLTree, and are linked together in a linked
> +    // list, using equalSize.
> +    void* pointer;
> +    size_t size;
> +    FreeListEntry* equalSize;

Should equalSize just be called "next" or "nextEntry", since it's a link in a
list?

+// Used to reverse sort an array of FreeListEntry pointers.
+static int sortFreeListEntriesByPointer(const void* leftPtr, const void*
rightPtr)

Maybe call this "reverseSortFreeListEntriesByPointer" or
"sortFreeListEntriesByPointerAscending" (Descending?).

+// Used to reverse sort an array of pointers.
+static int sortCommonSizedAllocations(const void* leftPtr, const void*
rightPtr)

Same here.

+	 if (FreeListEntry* entryInFreeList = freeList.search(entry->size,
freeList.EQUAL)) {
+	     // Find the last entry in the chain of entries already in the
freeList.
+	     while (FreeListEntry* next = entryInFreeList->equalSize)
+		 entryInFreeList = next;

Would it be better to add to the head of the list instead of the tail, to avoid
having to walk the whole list?

+    // instead we we periodically perform a sweep to try to coalesce
neigboring

Typo: "we we"

+    size_t countFreedSiceLastCoalesce;

Typo: Should be "Since".

+	     // Remove the entry from the freeList.  But! -
+	     // Each entry in the tree may represent a chain of multiple chunks
of the
+	     // same size, and we only want to remove one on them.  So, if this
entry
+	     // does have a chain, just snip the last one off the end.

How about popping from the head instead of the tail? (Same comment as above.)

+		 // There is memory left over, and it is large than the common
size.

Typo: Should be "larger".

+    // All allocations must be a multiple of the commonSize
+    // specified to the constructor.

I guess this comment is not true?

+		 // There is memory left over, and it is large than the common
size.
+		 // We can reuse the existing FreeListEntry node to add this
back

I think this case is hit if the memory left over is smaller than the common
size, too, right?

r=me