Title: [155395] trunk/Source/_javascript_Core
- Revision
- 155395
- Author
- [email protected]
- Date
- 2013-09-09 15:41:00 -0700 (Mon, 09 Sep 2013)
Log Message
JSArray::shiftCountWithArrayStorage doesn't change indexBias when shifting the last element in m_vector
https://bugs.webkit.org/show_bug.cgi?id=120389
Reviewed by Michael Saboff.
Went through and cleaned up shiftCountWithArrayStorage. Gave meaningful variable names
and commented the confusing parts. This led to realizing how to fix this bug, which has
been done. The issue was that we were modifying the vector length unconditionally, even
when we weren't logically changing the length of the vector. Instead, we should only modify
the vector length when we modify the index bias.
* runtime/JSArray.cpp:
(JSC::JSArray::shiftCountWithArrayStorage):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (155394 => 155395)
--- trunk/Source/_javascript_Core/ChangeLog 2013-09-09 22:36:50 UTC (rev 155394)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-09-09 22:41:00 UTC (rev 155395)
@@ -1,3 +1,19 @@
+2013-08-29 Mark Hahnenberg <[email protected]>
+
+ JSArray::shiftCountWithArrayStorage doesn't change indexBias when shifting the last element in m_vector
+ https://bugs.webkit.org/show_bug.cgi?id=120389
+
+ Reviewed by Michael Saboff.
+
+ Went through and cleaned up shiftCountWithArrayStorage. Gave meaningful variable names
+ and commented the confusing parts. This led to realizing how to fix this bug, which has
+ been done. The issue was that we were modifying the vector length unconditionally, even
+ when we weren't logically changing the length of the vector. Instead, we should only modify
+ the vector length when we modify the index bias.
+
+ * runtime/JSArray.cpp:
+ (JSC::JSArray::shiftCountWithArrayStorage):
+
2013-09-08 Anders Carlsson <[email protected]>
Begin moving off of TypeTraits.h
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (155394 => 155395)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2013-09-09 22:36:50 UTC (rev 155394)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2013-09-09 22:41:00 UTC (rev 155395)
@@ -55,6 +55,8 @@
static const size_t largeHeapSize = 32 * MB; // About 1.5X the average webpage.
static const size_t smallHeapSize = 1 * MB; // Matches the FastMalloc per-thread cache.
+#define ENABLE_GC_LOGGING 1
+
#if ENABLE(GC_LOGGING)
#if COMPILER(CLANG)
#define DEFINE_GC_LOGGING_GLOBAL(type, name, arguments) \
Modified: trunk/Source/_javascript_Core/runtime/JSArray.cpp (155394 => 155395)
--- trunk/Source/_javascript_Core/runtime/JSArray.cpp 2013-09-09 22:36:50 UTC (rev 155394)
+++ trunk/Source/_javascript_Core/runtime/JSArray.cpp 2013-09-09 22:41:00 UTC (rev 155395)
@@ -700,29 +700,50 @@
unsigned usedVectorLength = min(vectorLength, oldLength);
- vectorLength -= count;
- storage->setVectorLength(vectorLength);
-
- if (vectorLength) {
- if (startIndex < usedVectorLength - (startIndex + count)) {
- if (startIndex) {
- memmove(
- storage->m_vector + count,
- storage->m_vector,
- sizeof(JSValue) * startIndex);
- }
- m_butterfly = m_butterfly->shift(structure(), count);
- storage = m_butterfly->arrayStorage();
- storage->m_indexBias += count;
- } else {
+ unsigned numElementsBeforeShiftRegion = startIndex;
+ unsigned firstIndexAfterShiftRegion = startIndex + count;
+ unsigned numElementsAfterShiftRegion = usedVectorLength - firstIndexAfterShiftRegion;
+ ASSERT(numElementsBeforeShiftRegion + count + numElementsAfterShiftRegion == usedVectorLength);
+
+ // The point of this comparison seems to be to minimize the amount of elements that have to
+ // be moved during a shift operation.
+ if (numElementsBeforeShiftRegion < numElementsAfterShiftRegion) {
+ // The number of elements before the shift region is less than the number of elements
+ // after the shift region, so we move the elements before to the right.
+ if (numElementsBeforeShiftRegion) {
+ RELEASE_ASSERT(count + startIndex <= vectorLength);
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);
}
+ // Adjust the Butterfly and the index bias. We only need to do this here because we're changing
+ // the start of the Butterfly, which needs to point at the first indexed property in the used
+ // portion of the vector.
+ m_butterfly = m_butterfly->shift(structure(), count);
+ storage = m_butterfly->arrayStorage();
+ storage->m_indexBias += count;
+
+ // Since we're consuming part of the vector by moving its beginning to the left,
+ // we need to modify the vector length appropriately.
+ storage->setVectorLength(vectorLength - count);
+ } else {
+ // The number of elements before the shift region is greater than or equal to the number
+ // of elements after the shift region, so we move the elements after the shift region to the left.
+ memmove(
+ storage->m_vector + startIndex,
+ storage->m_vector + firstIndexAfterShiftRegion,
+ sizeof(JSValue) * numElementsAfterShiftRegion);
+ // Clear the slots of the elements we just moved.
+ unsigned startOfEmptyVectorTail = usedVectorLength - count;
+ for (unsigned i = startOfEmptyVectorTail; i < usedVectorLength; ++i)
+ storage->m_vector[i].clear();
+ // We don't modify the index bias or the Butterfly pointer in this case because we're not changing
+ // the start of the Butterfly, which needs to point at the first indexed property in the used
+ // portion of the vector. We also don't modify the vector length because we're not actually changing
+ // its length; we're just using less of it.
}
+
return true;
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
