[webkit-reviews] review denied: [Bug 94810] Vector::shrinkToFit should use realloc when canCopyWithMemcpy is true : [Attachment 161042] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Yong Li <yoli at rim.com>'s
request for review:
Bug 94810: Vector::shrinkToFit should use realloc when canCopyWithMemcpy is
true
https://bugs.webkit.org/show_bug.cgi?id=94810

Attachment 161042: Patch
https://bugs.webkit.org/attachment.cgi?id=161042&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=161042&action=review


I just r- for the minor issues, but everything looks good really. 

I'll have to double check the VectorTraits<T>::canMoveWithMemcpy, I do not
remember by heart if it is the right one.

> Source/WTF/ChangeLog:8
> +	   Only tested on BlackBerry. So it is wrapped with
PLATFORM(BLACKBERRY) at the mean time.

"at the mean time" -> "in the meantime"/"for now"

> Source/WTF/ChangeLog:12
> +	   Use realloc to shrink buffer when it is changing from a large malloc
buffer
> +	   to another large one (> 1k), and canMoveWithMemcpy is true. When a
Vector<char> shrinks
> +	   capacity from 2K to 1K, this gives more than 10x times performance
boost on shrinkToFit()
> +	   (Tested on BlackBerry).

This is outdated, your removed the 1K limit.
Please use a new paragraph for the performance detail, that'll make it easier
to track.

10 times faster!!! I should really try your bench with fastMalloc!

> Source/WTF/wtf/Vector.h:288
> +	       // FIXME: Return true when realloc() can give better
performance.

The "when" is ambiguous IMHO. More like:
// FIXME: Return true on the platform where realloc() give better performance.
?

+ UNUSED_PARAM(newCapacity); (for the Mac break).