[webkit-reviews] review granted: [Bug 15606] make cut-off for sparse vs. dense arrays smarter for speed with large arrays : [Attachment 16784] patch with change log

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 15606: make cut-off for sparse vs. dense arrays smarter for speed with
large arrays
http://bugs.webkit.org/show_bug.cgi?id=15606

Attachment 16784: patch with change log
http://bugs.webkit.org/attachment.cgi?id=16784&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
Regarding this FIXME. The extra copy is necessary because mergesort modifies
the buffer that you pass it as it works, and there are times when not all
values are in the buffer, some are only in auxiliary storage. But the compare
function can trigger a garbage collection. If a GC happens while the array's
storage pointer points to a buffer that does not contain all the values, then
some objects that should be live will be trashed. This led to real crashes
before I added the copy, so it's not just a problem in theory. I would suggest
replacing the FIXME with a comment that explains this. My apologies for not
adding such a comment at the time.

+	 // FIXME: Why do we need this copy of the array storage?
+	 // I know that mergesort allocates more memory internally,
+	 // but I don't see any use of the original m_storage here!
+	 size_t size = storageSize(m_vectorLength);
+	 ArrayStorage* copy = static_cast<ArrayStorage*>(fastMalloc(size));
+	 memcpy(copy, m_storage, size);
+	 mergesort(copy->m_vector, lengthNotIncludingUndefined,
sizeof(JSValue*), compareWithCompareFunctionForQSort);
+	 fastFree(m_storage);
+	 m_storage = copy;

Everything else looks good. r=me