[webkit-reviews] review granted: [Bug 82012] BitVector::resizeOutOfLine doesn't memset when converting an inline buffer : [Attachment 133427] Fixes this annoying bug

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 82012: BitVector::resizeOutOfLine doesn't memset when converting an inline
buffer
https://bugs.webkit.org/show_bug.cgi?id=82012

Attachment 133427: Fixes this annoying bug
https://bugs.webkit.org/attachment.cgi?id=133427&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=133427&action=review


R=me.  Just make sure you test the Mac port (particularly fast/js, and do a run
of the V8 benchmarks using the V8 harness) in addition to Chromium, since JSC
uses BitVector in a couple places.

>>>>>> Source/JavaScriptCore/wtf/BitVector.cpp:96
>>>>>> +	memset(newOutOfLineBits->bits() + 1, 0, (newNumWords - 1) *
sizeof(void*));
>>>>> 
>>>>> It's not immediately clear to me that this is right.  Are you trying to
init the whole vector?	Or just the non-previously-inline bits?  What's the
magical +1/-1?
>>>> 
>>>> See the line above. We're copying the inline buffer :) So we shouldn't be
zero'ing that out.
>>> 
>>> Then shouldn't we be getting our "word count" from our "bit count"?  Or
alternatively, we could calloc our new vector before we copy.
>> 
>> i.e. non-previously-inline bits.
> 
> I don't quite follow. The buffer is initialized at allocation time. calloc
would work but we're using fastMalloc here. Do we have fastCalloc?

I think what Ryosuke doing is optimal. In most cases we're resizing the vector
by only a bit. So we allocate an uninitialized buffer, initialize the first
word by copying, and then initialize the remaining words with memset. LGTM.

> Source/JavaScriptCore/wtf/BitVector.h:107
> +    WTF_EXPORT_PRIVATE void resize(size_t numBits);

I'm OK with this change as well; no point in making this a separate patch.