82012 2012-03-22 21:41:58 -0700 BitVector::resizeOutOfLine doesn't memset when converting an inline buffer 2012-03-23 00:14:53 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 81737 1 rniwa rniwa barraclough darin eric fpizlo mjs sam webkit.review.bot oldest_to_newest 586076 0 rniwa 2012-03-22 21:41:58 -0700 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. 586077 1 133427 rniwa 2012-03-22 21:45:25 -0700 Created attachment 133427 Fixes this annoying bug 586109 2 133427 eric 2012-03-22 23:21:49 -0700 Comment on attachment 133427 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? 586117 3 rniwa 2012-03-22 23:31:41 -0700 (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. 586119 4 133427 eric 2012-03-22 23:34:02 -0700 Comment on attachment 133427 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. 586120 5 133427 rniwa 2012-03-22 23:34:27 -0700 Comment on attachment 133427 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. 586123 6 rniwa 2012-03-22 23:40:03 -0700 (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? 586124 7 133427 fpizlo 2012-03-22 23:44:53 -0700 Comment on attachment 133427 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. 586126 8 133427 eric 2012-03-22 23:47:54 -0700 Comment on attachment 133427 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. :) 586127 9 eric 2012-03-22 23:50:06 -0700 (I'm not so much seeking to evangelize a position as I am to question the clarity of the patch as written.) 586131 10 fpizlo 2012-03-22 23:57:00 -0700 > 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. 586135 11 rniwa 2012-03-23 00:03:20 -0700 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). 586141 12 133427 webkit.review.bot 2012-03-23 00:14:48 -0700 Comment on attachment 133427 Fixes this annoying bug Clearing flags on attachment: 133427 Committed r111834: <http://trac.webkit.org/changeset/111834> 586142 13 webkit.review.bot 2012-03-23 00:14:53 -0700 All reviewed patches have been landed. Closing bug. 133427 2012-03-22 21:45:25 -0700 2012-03-23 00:14:47 -0700 Fixes this annoying bug fix82012 text/plain 3470 rniwa SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvQml0VmVjdG9yLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL0JpdFZlY3Rvci5jcHAJKHJldmlzaW9uIDEx MTcxNCkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvQml0VmVjdG9yLmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtODksMTMgKzg5LDE0IEBACiB7CiAgICAgQVNTRVJUKG51bUJpdHMgPiBtYXhJ bmxpbmVCaXRzKCkpOwogICAgIE91dE9mTGluZUJpdHMqIG5ld091dE9mTGluZUJpdHMgPSBPdXRP ZkxpbmVCaXRzOjpjcmVhdGUobnVtQml0cyk7CisgICAgc2l6ZV90IG5ld051bVdvcmRzID0gbmV3 T3V0T2ZMaW5lQml0cy0+bnVtV29yZHMoKTsKICAgICBpZiAoaXNJbmxpbmUoKSkgewogICAgICAg ICAvLyBNYWtlIHN1cmUgdGhhdCBhbGwgb2YgdGhlIGJpdHMgYXJlIHplcm8gaW4gY2FzZSB3ZSBk byBhIG5vLW9wIHJlc2l6ZS4KICAgICAgICAgKm5ld091dE9mTGluZUJpdHMtPmJpdHMoKSA9IG1f Yml0c09yUG9pbnRlciAmIH4oc3RhdGljX2Nhc3Q8dWludHB0cl90PigxKSA8PCBtYXhJbmxpbmVC aXRzKCkpOworICAgICAgICBtZW1zZXQobmV3T3V0T2ZMaW5lQml0cy0+Yml0cygpICsgMSwgMCwg KG5ld051bVdvcmRzIC0gMSkgKiBzaXplb2Yodm9pZCopKTsKICAgICB9IGVsc2UgewogICAgICAg ICBpZiAobnVtQml0cyA+IHNpemUoKSkgewogICAgICAgICAgICAgc2l6ZV90IG9sZE51bVdvcmRz ID0gb3V0T2ZMaW5lQml0cygpLT5udW1Xb3JkcygpOwotICAgICAgICAgICAgc2l6ZV90IG5ld051 bVdvcmRzID0gbmV3T3V0T2ZMaW5lQml0cy0+bnVtV29yZHMoKTsKICAgICAgICAgICAgIG1lbWNw eShuZXdPdXRPZkxpbmVCaXRzLT5iaXRzKCksIG91dE9mTGluZUJpdHMoKS0+Yml0cygpLCBvbGRO dW1Xb3JkcyAqIHNpemVvZih2b2lkKikpOwogICAgICAgICAgICAgbWVtc2V0KG5ld091dE9mTGlu ZUJpdHMtPmJpdHMoKSArIG9sZE51bVdvcmRzLCAwLCAobmV3TnVtV29yZHMgLSBvbGROdW1Xb3Jk cykgKiBzaXplb2Yodm9pZCopKTsKICAgICAgICAgfSBlbHNlCkluZGV4OiBTb3VyY2UvSmF2YVNj cmlwdENvcmUvd3RmL0JpdFZlY3Rvci5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS93dGYvQml0VmVjdG9yLmgJKHJldmlzaW9uIDExMTcxNCkKKysrIFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS93dGYvQml0VmVjdG9yLmgJKHdvcmtpbmcgY29weSkKQEAgLTEwNCw5ICsxMDQsOSBA QAogICAgIH0KICAgICAKICAgICAvLyBMaWtlIGVuc3VyZVNpemUoKSwgYnV0IHN1cHBvcnRzIHJl ZHVjaW5nIHRoZSBzaXplIG9mIHRoZSBiaXR2ZWN0b3IuCi0gICAgdm9pZCByZXNpemUoc2l6ZV90 IG51bUJpdHMpOworICAgIFdURl9FWFBPUlRfUFJJVkFURSB2b2lkIHJlc2l6ZShzaXplX3QgbnVt Qml0cyk7CiAgICAgCi0gICAgdm9pZCBjbGVhckFsbCgpOworICAgIFdURl9FWFBPUlRfUFJJVkFU RSB2b2lkIGNsZWFyQWxsKCk7CiAKICAgICBib29sIHF1aWNrR2V0KHNpemVfdCBiaXQpIGNvbnN0 CiAgICAgewpAQCAtMTkzLDkgKzE5Myw5IEBACiAgICAgICAgIHVpbnRwdHJfdCogYml0cygpIHsg cmV0dXJuIGJpdHdpc2VfY2FzdDx1aW50cHRyX3QqPih0aGlzICsgMSk7IH0KICAgICAgICAgY29u c3QgdWludHB0cl90KiBiaXRzKCkgY29uc3QgeyByZXR1cm4gYml0d2lzZV9jYXN0PGNvbnN0IHVp bnRwdHJfdCo+KHRoaXMgKyAxKTsgfQogICAgICAgICAKLSAgICAgICAgc3RhdGljIE91dE9mTGlu ZUJpdHMqIGNyZWF0ZShzaXplX3QgbnVtQml0cyk7CisgICAgICAgIHN0YXRpYyBXVEZfRVhQT1JU X1BSSVZBVEUgT3V0T2ZMaW5lQml0cyogY3JlYXRlKHNpemVfdCBudW1CaXRzKTsKICAgICAgICAg Ci0gICAgICAgIHN0YXRpYyB2b2lkIGRlc3Ryb3koT3V0T2ZMaW5lQml0cyopOworICAgICAgICBz dGF0aWMgV1RGX0VYUE9SVF9QUklWQVRFIHZvaWQgZGVzdHJveShPdXRPZkxpbmVCaXRzKik7CiAK ICAgICBwcml2YXRlOgogICAgICAgICBPdXRPZkxpbmVCaXRzKHNpemVfdCBudW1CaXRzKQpAQCAt MjExLDggKzIxMSw4IEBACiAgICAgY29uc3QgT3V0T2ZMaW5lQml0cyogb3V0T2ZMaW5lQml0cygp IGNvbnN0IHsgcmV0dXJuIGJpdHdpc2VfY2FzdDxjb25zdCBPdXRPZkxpbmVCaXRzKj4obV9iaXRz T3JQb2ludGVyIDw8IDEpOyB9CiAgICAgT3V0T2ZMaW5lQml0cyogb3V0T2ZMaW5lQml0cygpIHsg cmV0dXJuIGJpdHdpc2VfY2FzdDxPdXRPZkxpbmVCaXRzKj4obV9iaXRzT3JQb2ludGVyIDw8IDEp OyB9CiAgICAgCi0gICAgdm9pZCByZXNpemVPdXRPZkxpbmUoc2l6ZV90IG51bUJpdHMpOwotICAg IHZvaWQgc2V0U2xvdyhjb25zdCBCaXRWZWN0b3ImIG90aGVyKTsKKyAgICBXVEZfRVhQT1JUX1BS SVZBVEUgdm9pZCByZXNpemVPdXRPZkxpbmUoc2l6ZV90IG51bUJpdHMpOworICAgIFdURl9FWFBP UlRfUFJJVkFURSB2b2lkIHNldFNsb3coY29uc3QgQml0VmVjdG9yJiBvdGhlcik7CiAgICAgCiAg ICAgdWludHB0cl90KiBiaXRzKCkKICAgICB7CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUv Q2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cJ KHJldmlzaW9uIDExMTgyNSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cJKHdv cmtpbmcgY29weSkKQEAgLTEsMyArMSwxOCBAQAorMjAxMi0wMy0yMiAgUnlvc3VrZSBOaXdhICA8 cm5pd2FAd2Via2l0Lm9yZz4KKworICAgICAgICBCaXRWZWN0b3I6OnJlc2l6ZU91dE9mTGluZSBk b2Vzbid0IG1lbXNldCB3aGVuIGNvbnZlcnRpbmcgYW4gaW5saW5lIGJ1ZmZlcgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODIwMTIKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbml0aWFsaXplIG91dC1vZi1s aW5lIGJ1ZmZlcnMgd2hpbGUgZXh0ZW5kaW5nIGFuIGlubGluZSBidWZmZXIuIEFsc28gZXhwb3J0 IHN5bWJvbHMgdG8gYmUgdXNlZCBpbiBXZWJDb3JlLgorCisgICAgICAgICogd3RmL0JpdFZlY3Rv ci5jcHA6CisgICAgICAgIChXVEY6OkJpdFZlY3Rvcjo6cmVzaXplT3V0T2ZMaW5lKToKKyAgICAg ICAgKiB3dGYvQml0VmVjdG9yLmg6CisgICAgICAgIChCaXRWZWN0b3IpOgorICAgICAgICAoT3V0 T2ZMaW5lQml0cyk6CisKIDIwMTItMDMtMjIgIEplc3NpZSBCZXJsaW4gIDxqYmVybGluQGFwcGxl LmNvbT4KIAogICAgICAgICBXaW5kb3dzIGJ1aWxkIGZpeCBhZnRlciByMTExNzc4Lgo=