Commit b5e07304 authored by fpizlo@apple.com's avatar fpizlo@apple.com

Address a FIXME in JSArray::sort

https://bugs.webkit.org/show_bug.cgi?id=98080
<rdar://problem/12407844>

Reviewed by Oliver Hunt.

Source/JavaScriptCore: 

Get rid of fast sorting of sparse maps. I don't know that it's broken but I do know that we don't
have coverage for it. Then also address the FIXME in JSArray::sort regarding side-effecting
compare functions.

* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncSort):
* runtime/JSArray.cpp:
(JSC::JSArray::sortNumeric):
(JSC::JSArray::sort):
(JSC::JSArray::compactForSorting):
* runtime/JSArray.h:
(JSArray):
* runtime/JSObject.h:
(JSC::JSObject::hasSparseMap):
(JSObject):

LayoutTests: 

* fast/js/jsc-test-list:
* fast/js/script-tests/sort-with-side-effecting-comparisons.js: Added.
* fast/js/sort-with-side-effecting-comparisons-expected.txt: Added.
* fast/js/sort-with-side-effecting-comparisons.html: Added.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@130102 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent d2f9b5aa
2012-10-01 Filip Pizlo <fpizlo@apple.com>
Address a FIXME in JSArray::sort
https://bugs.webkit.org/show_bug.cgi?id=98080
<rdar://problem/12407844>
Reviewed by Oliver Hunt.
* fast/js/jsc-test-list:
* fast/js/script-tests/sort-with-side-effecting-comparisons.js: Added.
* fast/js/sort-with-side-effecting-comparisons-expected.txt: Added.
* fast/js/sort-with-side-effecting-comparisons.html: Added.
2012-10-01 Roger Fong <roger_fong@apple.com> 2012-10-01 Roger Fong <roger_fong@apple.com>
Unreviewed. Skipping flaky test on Windows: inspector/debugger/dynamic-script-tag.html. Unreviewed. Skipping flaky test on Windows: inspector/debugger/dynamic-script-tag.html.
...@@ -297,6 +297,7 @@ fast/js/sort-no-jit-code-crash ...@@ -297,6 +297,7 @@ fast/js/sort-no-jit-code-crash
fast/js/sort-non-numbers fast/js/sort-non-numbers
fast/js/sort-randomly fast/js/sort-randomly
fast/js/sort-stability fast/js/sort-stability
fast/js/sort-with-side-effecting-comparisons
fast/js/sparse-array fast/js/sparse-array
fast/js/stack-overflow-arrity-catch fast/js/stack-overflow-arrity-catch
fast/js/stack-overflow-catch fast/js/stack-overflow-catch
......
description(
"Checks that sorting an array with a side-effecting comparison function doesn't trigger assertions."
);
var array = [];
for (var i = 0; i < 20000; ++i)
array.push(i);
array.sort(function(a, b) {
array.shift();
if (a < b)
return -1;
if (a > b)
return 1;
return 0;
});
testPassed("It worked.");
Checks that sorting an array with a side-effecting comparison function doesn't trigger assertions.
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS It worked.
PASS successfullyParsed is true
TEST COMPLETE
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<script src="resources/js-test-pre.js"></script>
</head>
<body>
<script src="script-tests/sort-with-side-effecting-comparisons.js"></script>
<script src="resources/js-test-post.js"></script>
</body>
</html>
2012-10-01 Filip Pizlo <fpizlo@apple.com>
Address a FIXME in JSArray::sort
https://bugs.webkit.org/show_bug.cgi?id=98080
<rdar://problem/12407844>
Reviewed by Oliver Hunt.
Get rid of fast sorting of sparse maps. I don't know that it's broken but I do know that we don't
have coverage for it. Then also address the FIXME in JSArray::sort regarding side-effecting
compare functions.
* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncSort):
* runtime/JSArray.cpp:
(JSC::JSArray::sortNumeric):
(JSC::JSArray::sort):
(JSC::JSArray::compactForSorting):
* runtime/JSArray.h:
(JSArray):
* runtime/JSObject.h:
(JSC::JSObject::hasSparseMap):
(JSObject):
2012-10-01 Jonathan Liu <net147@gmail.com> 2012-10-01 Jonathan Liu <net147@gmail.com>
Remove unused sys/mman.h include Remove unused sys/mman.h include
......
...@@ -655,7 +655,7 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncSort(ExecState* exec) ...@@ -655,7 +655,7 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncSort(ExecState* exec)
CallData callData; CallData callData;
CallType callType = getCallData(function, callData); CallType callType = getCallData(function, callData);
if (thisObj->classInfo() == &JSArray::s_info && !asArray(thisObj)->inSparseIndexingMode() && !shouldUseSlowPut(thisObj->structure()->indexingType())) { if (thisObj->classInfo() == &JSArray::s_info && !asArray(thisObj)->hasSparseMap() && !shouldUseSlowPut(thisObj->structure()->indexingType())) {
if (isNumericCompareFunction(exec, callType, callData)) if (isNumericCompareFunction(exec, callType, callData))
asArray(thisObj)->sortNumeric(exec, function, callType, callData); asArray(thisObj)->sortNumeric(exec, function, callType, callData);
else if (callType != CallTypeNone) else if (callType != CallTypeNone)
......
...@@ -601,13 +601,10 @@ void JSArray::sortNumeric(ExecState* exec, JSValue compareFunction, CallType cal ...@@ -601,13 +601,10 @@ void JSArray::sortNumeric(ExecState* exec, JSValue compareFunction, CallType cal
return; return;
case ArrayWithArrayStorage: { case ArrayWithArrayStorage: {
unsigned lengthNotIncludingUndefined = compactForSorting(exec->globalData()); unsigned lengthNotIncludingUndefined = compactForSorting();
ArrayStorage* storage = m_butterfly->arrayStorage(); ArrayStorage* storage = m_butterfly->arrayStorage();
if (storage->m_sparseMap.get()) { ASSERT(!storage->m_sparseMap);
throwOutOfMemoryError(exec);
return;
}
if (!lengthNotIncludingUndefined) if (!lengthNotIncludingUndefined)
return; return;
...@@ -646,12 +643,9 @@ void JSArray::sort(ExecState* exec) ...@@ -646,12 +643,9 @@ void JSArray::sort(ExecState* exec)
return; return;
case ArrayWithArrayStorage: { case ArrayWithArrayStorage: {
unsigned lengthNotIncludingUndefined = compactForSorting(exec->globalData()); unsigned lengthNotIncludingUndefined = compactForSorting();
ArrayStorage* storage = m_butterfly->arrayStorage(); ArrayStorage* storage = m_butterfly->arrayStorage();
if (storage->m_sparseMap.get()) { ASSERT(!storage->m_sparseMap);
throwOutOfMemoryError(exec);
return;
}
if (!lengthNotIncludingUndefined) if (!lengthNotIncludingUndefined)
return; return;
...@@ -811,18 +805,17 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType, ...@@ -811,18 +805,17 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType,
return; return;
case ArrayWithArrayStorage: { case ArrayWithArrayStorage: {
ArrayStorage* storage = m_butterfly->arrayStorage();
// FIXME: This ignores exceptions raised in the compare function or in toNumber. // FIXME: This ignores exceptions raised in the compare function or in toNumber.
// The maximum tree depth is compiled in - but the caller is clearly up to no good // The maximum tree depth is compiled in - but the caller is clearly up to no good
// if a larger array is passed. // if a larger array is passed.
ASSERT(storage->length() <= static_cast<unsigned>(std::numeric_limits<int>::max())); ASSERT(arrayStorage()->length() <= static_cast<unsigned>(std::numeric_limits<int>::max()));
if (storage->length() > static_cast<unsigned>(std::numeric_limits<int>::max())) if (arrayStorage()->length() > static_cast<unsigned>(std::numeric_limits<int>::max()))
return; return;
unsigned usedVectorLength = min(storage->length(), storage->vectorLength()); unsigned usedVectorLength = min(arrayStorage()->length(), arrayStorage()->vectorLength());
unsigned nodeCount = usedVectorLength + (storage->m_sparseMap ? storage->m_sparseMap->size() : 0); ASSERT(!arrayStorage()->m_sparseMap);
unsigned nodeCount = usedVectorLength;
if (!nodeCount) if (!nodeCount)
return; return;
...@@ -850,14 +843,18 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType, ...@@ -850,14 +843,18 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType,
// Iterate over the array, ignoring missing values, counting undefined ones, and inserting all other ones into the tree. // Iterate over the array, ignoring missing values, counting undefined ones, and inserting all other ones into the tree.
for (; numDefined < usedVectorLength; ++numDefined) { for (; numDefined < usedVectorLength; ++numDefined) {
JSValue v = storage->m_vector[numDefined].get(); if (numDefined > arrayStorage()->vectorLength())
break;
JSValue v = arrayStorage()->m_vector[numDefined].get();
if (!v || v.isUndefined()) if (!v || v.isUndefined())
break; break;
tree.abstractor().m_nodes[numDefined].value = v; tree.abstractor().m_nodes[numDefined].value = v;
tree.insert(numDefined); tree.insert(numDefined);
} }
for (unsigned i = numDefined; i < usedVectorLength; ++i) { for (unsigned i = numDefined; i < usedVectorLength; ++i) {
JSValue v = storage->m_vector[i].get(); if (i > arrayStorage()->vectorLength())
break;
JSValue v = arrayStorage()->m_vector[i].get();
if (v) { if (v) {
if (v.isUndefined()) if (v.isUndefined())
++numUndefined; ++numUndefined;
...@@ -871,51 +868,34 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType, ...@@ -871,51 +868,34 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType,
unsigned newUsedVectorLength = numDefined + numUndefined; unsigned newUsedVectorLength = numDefined + numUndefined;
if (SparseArrayValueMap* map = storage->m_sparseMap.get()) { ASSERT(!arrayStorage()->m_sparseMap);
newUsedVectorLength += map->size();
if (newUsedVectorLength > storage->vectorLength()) {
// Check that it is possible to allocate an array large enough to hold all the entries.
if ((newUsedVectorLength > MAX_STORAGE_VECTOR_LENGTH) || !increaseVectorLength(exec->globalData(), newUsedVectorLength)) {
throwOutOfMemoryError(exec);
return;
}
storage = m_butterfly->arrayStorage();
}
SparseArrayValueMap::const_iterator end = map->end();
for (SparseArrayValueMap::const_iterator it = map->begin(); it != end; ++it) {
tree.abstractor().m_nodes[numDefined].value = it->second.getNonSparseMode();
tree.insert(numDefined);
++numDefined;
}
deallocateSparseIndexMap();
}
ASSERT(tree.abstractor().m_nodes.size() >= numDefined); // The array size may have changed. Figure out the new bounds.
unsigned newestUsedVectorLength = min(arrayStorage()->length(), arrayStorage()->vectorLength());
// FIXME: If the compare function changed the length of the array, the following might be unsigned elementsToExtractThreshold = min(min(newestUsedVectorLength, numDefined), static_cast<unsigned>(tree.abstractor().m_nodes.size()));
// modifying the vector incorrectly. unsigned undefinedElementsThreshold = min(newestUsedVectorLength, newUsedVectorLength);
unsigned clearElementsThreshold = min(newestUsedVectorLength, usedVectorLength);
// Copy the values back into m_storage. // Copy the values back into m_storage.
AVLTree<AVLTreeAbstractorForArrayCompare, 44>::Iterator iter; AVLTree<AVLTreeAbstractorForArrayCompare, 44>::Iterator iter;
iter.start_iter_least(tree); iter.start_iter_least(tree);
JSGlobalData& globalData = exec->globalData(); JSGlobalData& globalData = exec->globalData();
for (unsigned i = 0; i < numDefined; ++i) { for (unsigned i = 0; i < elementsToExtractThreshold; ++i) {
storage->m_vector[i].set(globalData, this, tree.abstractor().m_nodes[*iter].value); arrayStorage()->m_vector[i].set(globalData, this, tree.abstractor().m_nodes[*iter].value);
++iter; ++iter;
} }
// Put undefined values back in. // Put undefined values back in.
for (unsigned i = numDefined; i < newUsedVectorLength; ++i) for (unsigned i = elementsToExtractThreshold; i < undefinedElementsThreshold; ++i)
storage->m_vector[i].setUndefined(); arrayStorage()->m_vector[i].setUndefined();
// Ensure that unused values in the vector are zeroed out. // Ensure that unused values in the vector are zeroed out.
for (unsigned i = newUsedVectorLength; i < usedVectorLength; ++i) for (unsigned i = undefinedElementsThreshold; i < clearElementsThreshold; ++i)
storage->m_vector[i].clear(); arrayStorage()->m_vector[i].clear();
storage->m_numValuesInVector = newUsedVectorLength;
arrayStorage()->m_numValuesInVector = undefinedElementsThreshold;
return; return;
} }
...@@ -982,7 +962,7 @@ void JSArray::copyToArguments(ExecState* exec, CallFrame* callFrame, uint32_t le ...@@ -982,7 +962,7 @@ void JSArray::copyToArguments(ExecState* exec, CallFrame* callFrame, uint32_t le
} }
} }
unsigned JSArray::compactForSorting(JSGlobalData& globalData) unsigned JSArray::compactForSorting()
{ {
ASSERT(!inSparseIndexingMode()); ASSERT(!inSparseIndexingMode());
...@@ -1016,23 +996,7 @@ unsigned JSArray::compactForSorting(JSGlobalData& globalData) ...@@ -1016,23 +996,7 @@ unsigned JSArray::compactForSorting(JSGlobalData& globalData)
unsigned newUsedVectorLength = numDefined + numUndefined; unsigned newUsedVectorLength = numDefined + numUndefined;
if (SparseArrayValueMap* map = storage->m_sparseMap.get()) { ASSERT(!storage->m_sparseMap);
newUsedVectorLength += map->size();
if (newUsedVectorLength > storage->vectorLength()) {
// Check that it is possible to allocate an array large enough to hold all the entries - if not,
// exception is thrown by caller.
if ((newUsedVectorLength > MAX_STORAGE_VECTOR_LENGTH) || !increaseVectorLength(globalData, newUsedVectorLength))
return 0;
storage = m_butterfly->arrayStorage();
}
SparseArrayValueMap::const_iterator end = map->end();
for (SparseArrayValueMap::const_iterator it = map->begin(); it != end; ++it)
storage->m_vector[numDefined++].setWithoutWriteBarrier(it->second.getNonSparseMode());
deallocateSparseIndexMap();
}
for (unsigned i = numDefined; i < newUsedVectorLength; ++i) for (unsigned i = numDefined; i < newUsedVectorLength; ++i)
storage->m_vector[i].setUndefined(); storage->m_vector[i].setUndefined();
......
...@@ -103,7 +103,7 @@ namespace JSC { ...@@ -103,7 +103,7 @@ namespace JSC {
bool unshiftCountSlowCase(JSGlobalData&, bool, unsigned); bool unshiftCountSlowCase(JSGlobalData&, bool, unsigned);
unsigned compactForSorting(JSGlobalData&); unsigned compactForSorting();
}; };
inline Butterfly* createArrayButterfly(JSGlobalData& globalData, unsigned initialLength) inline Butterfly* createArrayButterfly(JSGlobalData& globalData, unsigned initialLength)
......
...@@ -327,6 +327,19 @@ namespace JSC { ...@@ -327,6 +327,19 @@ namespace JSC {
} }
} }
bool hasSparseMap()
{
switch (structure()->indexingType()) {
case ALL_BLANK_INDEXING_TYPES:
return false;
case ALL_ARRAY_STORAGE_INDEXING_TYPES:
return m_butterfly->arrayStorage()->m_sparseMap;
default:
ASSERT_NOT_REACHED();
return false;
}
}
bool inSparseIndexingMode() bool inSparseIndexingMode()
{ {
switch (structure()->indexingType()) { switch (structure()->indexingType()) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment