Bug 82012 – BitVector::resizeOutOfLine doesn't memset when converting an inline buffer

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ryosuke Niwa]

BitVector::resizeOutOfLine doesn't call memset on the new out-of-line buffer when the old buffer was inline, resulting in a bit vector with uninitialized entries.

[Ryosuke Niwa]

Created attachment 133427 [details]
Fixes this annoying bug

[Eric Seidel (no email)]

Comment on attachment 133427 [details]
Fixes this annoying bug

View in context: https://bugs.webkit.org/attachment.cgi?id=133427&action=review

Why the export changes (since nothign seems to be usign tehm yet?)

> 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?

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 133427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133427&action=review
> 
> Why the export changes (since nothign seems to be usign tehm yet?)

My patch for the bug 81737 will use it. I can revert that part if you want.

> > 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.

[Eric Seidel (no email)]

Comment on attachment 133427 [details]
Fixes this annoying bug

View in context: https://bugs.webkit.org/attachment.cgi?id=133427&action=review

>>> 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.

[Ryosuke Niwa]

Comment on attachment 133427 [details]
Fixes this annoying bug

View in context: https://bugs.webkit.org/attachment.cgi?id=133427&action=review

>>> 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.

i.e. non-previously-inline bits.

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 133427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133427&action=review
> 
> >>> 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 don't quite follow. The buffer is initialized at allocation time. calloc would work but we're using fastMalloc here. Do we have fastCalloc?

[Filip Pizlo]

Comment on attachment 133427 [details]
Fixes this annoying bug

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.

[Eric Seidel (no email)]

Comment on attachment 133427 [details]
Fixes this annoying bug

View in context: https://bugs.webkit.org/attachment.cgi?id=133427&action=review

>>>>>>> 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.

Sure.  I'm not debating the correctness (yet), more debating the clarity.  We have a function "maxInlineBits()" which seems to suggest that the concept of the number of bits this vector supports is an abstract (and possibly mutable) concept.  This breaks that abstraction by assumign that we'll only ever have at most one word inline.

To answer your previous question, rniwa, yes we do have a fastCalloc.

I also doubt that initializing a single word twice would ever show up in any performance sample. :)  If it's more clear to do it that way (fewer magic constants), that seems like a win.  w/o seeing teh code I can't really comment if it's clearer or not. :)

[Eric Seidel (no email)]

(I'm not so much seeking to evangelize a position as I am to question the clarity of the patch as written.)

[Filip Pizlo]

> Sure.  I'm not debating the correctness (yet), more debating the clarity.  We have a function "maxInlineBits()" which seems to suggest that the concept of the number of bits this vector supports is an abstract (and possibly mutable) concept.  This breaks that abstraction by assumign that we'll only ever have at most one word inline.

It doesn't break the abstraction; that method sits behind the abstraction already, and already does almost exactly the same logic (copy things that need to be copied, memset things that need to be cleared) in the else case.

> 
> To answer your previous question, rniwa, yes we do have a fastCalloc.
> 
> I also doubt that initializing a single word twice would ever show up in any performance sample. :)  If it's more clear to do it that way (fewer magic constants), that seems like a win.  w/o seeing teh code I can't really comment if it's clearer or not. :)

I guess, the way I see it, the line being added here does not have magical constants, unless you consider "0", "1", and "sizeof(void*)" to be magical.

But whatever.  There are probably many ways of writing this code, and I'm just happy that this silly bug is fixed.

[Ryosuke Niwa]

I had a plan to templatize BitVector to support a larger inline buffer so maybe we can abstract 1 away when I (or someone else) do that work. As is, resizeOutOfLine doesn't work at all if inline buffer's size is more than sizeof(uintptr_t).

[WebKit Review Bot]

Comment on attachment 133427 [details]
Fixes this annoying bug

Clearing flags on attachment: 133427

Committed r111834: <http://trac.webkit.org/changeset/111834>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.