This leads to the JSArray forgetting it's true size (it thinks it has 8 bytes less than it actually does).
Created attachment 209836 [details] Patch
Comment on attachment 209836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209836&action=review > Source/JavaScriptCore/ChangeLog:8 > + This leads to the JSArray forgetting it's true size (it thinks it has 8 bytes less Should be "its true size".
(In reply to comment #2) > (From update of attachment 209836 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209836&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > + This leads to the JSArray forgetting it's true size (it thinks it has 8 bytes less > > Should be "its true size". Will fix.
Comment on attachment 209836 [details] Patch r=me - With change log fix.
Comment on attachment 209836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209836&action=review > Source/JavaScriptCore/runtime/JSArray.cpp:729 > + return true; > } > + > + storage->m_indexBias += count; > return true; This looks like it's introducing a new bug: the (startIndex < usedVectorLength - (startIndex + count)) == true case already adds count to indexBias. So now you're adding it twice.
Comment on attachment 209836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209836&action=review >> Source/JavaScriptCore/runtime/JSArray.cpp:729 >> return true; > > This looks like it's introducing a new bug: the (startIndex < usedVectorLength - (startIndex + count)) == true case already adds count to indexBias. So now you're adding it twice. I don't think so. There's an early return inside the if block.
(In reply to comment #6) > (From update of attachment 209836 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209836&action=review > > >> Source/JavaScriptCore/runtime/JSArray.cpp:729 > >> return true; > > > > This looks like it's introducing a new bug: the (startIndex < usedVectorLength - (startIndex + count)) == true case already adds count to indexBias. So now you're adding it twice. > > I don't think so. There's an early return inside the if block. But it does look like the other branch of the if-statement isn't modifying indexBias either, and it appears that it should.
Created attachment 209901 [details] Patch
Comment on attachment 209901 [details] Patch This patch is wrong. It breaks Facebook.
Created attachment 209997 [details] Patch
(In reply to comment #10) > Created an attachment (id=209997) [details] > Patch I went through the code and tried to figure out exactly what it was doing. As I went I added new variables that more accurately describe some of the intermediate calculations that are going on, which make the code much clearer. I also added comments to explain why we're doing certain things in certain places.
Comment on attachment 209997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209997&action=review > Source/JavaScriptCore/runtime/JSArray.cpp:726 > + // Since we're consuming part of the vector by moving its beginning to the left, ...beginning to the *right*
Comment on attachment 209997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209997&action=review > Source/JavaScriptCore/runtime/JSArray.cpp:717 > memmove( > - storage->m_vector + startIndex, > - storage->m_vector + startIndex + count, > - sizeof(JSValue) * (usedVectorLength - (startIndex + count))); > - for (unsigned i = usedVectorLength - count; i < usedVectorLength; ++i) > - storage->m_vector[i].clear(); > - } > + storage->m_vector + count, > + storage->m_vector, > + sizeof(JSValue) * startIndex); What about when the result of the shift will result with a vector length of 0. Copying in that case is wasteful and may also be wrong. > Source/JavaScriptCore/runtime/JSArray.cpp:735 > + memmove( > + storage->m_vector + startIndex, > + storage->m_vector + firstIndexAfterShiftRegion, > + sizeof(JSValue) * numElementsAfterShiftRegion); Ditto.
(In reply to comment #13) > (From update of attachment 209997 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209997&action=review > > > Source/JavaScriptCore/runtime/JSArray.cpp:717 > > memmove( > > - storage->m_vector + startIndex, > > - storage->m_vector + startIndex + count, > > - sizeof(JSValue) * (usedVectorLength - (startIndex + count))); > > - for (unsigned i = usedVectorLength - count; i < usedVectorLength; ++i) > > - storage->m_vector[i].clear(); > > - } > > + storage->m_vector + count, > > + storage->m_vector, > > + sizeof(JSValue) * startIndex); > > What about when the result of the shift will result with a vector length of 0. Copying in that case is wasteful and may also be wrong. Why create a special case for that? I ran benchmarks with this patch and it had no effect. > > > Source/JavaScriptCore/runtime/JSArray.cpp:735 > > + memmove( > > + storage->m_vector + startIndex, > > + storage->m_vector + firstIndexAfterShiftRegion, > > + sizeof(JSValue) * numElementsAfterShiftRegion); > > Ditto.
Comment on attachment 209997 [details] Patch r=me per our discussion. Please add RELEASE_ASSERTs to make sure that neither memmove will write past the end (first memmove) or beginning (second memmove).
Committed r155395: <http://trac.webkit.org/changeset/155395>
Comment on attachment 209997 [details] Patch Cleared review? from attachment 209997 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).