Bug 75588 – unshift/pop fifo may consume excessive memory

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

Array object commonly store data in a vector, consisting of a portion that is in use, a pre-capacity (m_indexBias) and a post-capacity (the delta between m_length and m_vectorLength).  Calls to pop with grow the post-capacity, and the current algorithm for increasePrefixVectorLength (used by unshift) will never stink the post-capacity, so a unshift/pop fifo may consume an inordinate amount of memory, whilst having a relatively small active length.

[Gavin Barraclough]

Created attachment 121188 [details]
Fix

[Gavin Barraclough]

Created attachment 121189 [details]
simple micro-benchmark

Since unshift is not covered by our standard benchmarks, I've written a trivial micro-benchmark.
Should no significant impact, 19.3s total runtime -> 19.2s.

[Early Warning System Bot]

Comment on attachment 121188 [details]
Fix

Attachment 121188 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11115011

[Gavin Barraclough]

Fixed in r104120.

[Csaba Osztrogonác]

Comment on attachment 121188 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=121188&action=review

> Source/JavaScriptCore/runtime/JSArray.cpp:827
> -void JSArray::unshiftCount(ExecState* exec, int count)
> +void JSArray::unshiftCount(ExecState* exec, unsigned count)

After this change the following assert is always true:
ASSERT(count >= 0);

It caused build breakage in debug mode:
cc1plus: warnings being treated as errors
../../../../Source/JavaScriptCore/runtime/JSArray.cpp: In member function ‘void JSC::JSArray::unshiftCount(JSC::ExecState*, unsigned int)’:
../../../../Source/JavaScriptCore/runtime/JSArray.cpp:831: error: comparison of unsigned expression >= 0 is always true
../../../../Source/JavaScriptCore/runtime/JSArray.cpp:832: error: comparison of unsigned expression >= 0 is always true

> Source/JavaScriptCore/runtime/JSArray.h:261
> +        unsigned m_indexBias; // The number of JSValue sized blocks before ArrayStorage.

After this change the following assert is always true:
ASSERT(m_indexBias >= 0);

It caused build breakage in debug mode:
cc1plus: warnings being treated as errors
../../../../Source/JavaScriptCore/runtime/JSArray.cpp: In member function ‘void JSC::JSArray::unshiftCount(JSC::ExecState*, unsigned int)’:
../../../../Source/JavaScriptCore/runtime/JSArray.cpp:831: error: comparison of unsigned expression >= 0 is always true
../../../../Source/JavaScriptCore/runtime/JSArray.cpp:832: error: comparison of unsigned expression >= 0 is always true

[Csaba Osztrogonác]

Buildfix landed in http://trac.webkit.org/changeset/104136