Bug 82012

Created attachment 133427 [details] Fixes this annoying bug

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?

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

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.

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.

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

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.

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

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

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

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

Comment on attachment 133427 [details] Fixes this annoying bug Clearing flags on attachment: 133427 Committed r111834: <http://trac.webkit.org/changeset/111834>

All reviewed patches have been landed. Closing bug.