Bug 75588 - unshift/pop fifo may consume excessive memory
Summary: unshift/pop fifo may consume excessive memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 75140
  Show dependency tree / graph
 
Reported: 2012-01-04 16:58 PST by Gavin Barraclough
Modified: 2012-01-05 11:35 PST (History)
1 user (show)

See Also:

Attachments
Fix (15.40 KB, patch)
2012-01-04 17:15 PST, Gavin Barraclough 		sam: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
simple micro-benchmark (5.85 KB, application/x-javascript)
2012-01-04 17:17 PST, Gavin Barraclough 		no flags Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2012-01-04 16:58:42 PST 
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.
Comment 1 Gavin Barraclough 2012-01-04 17:15:27 PST 
Created attachment 121188 [details]
Fix
Comment 2 Gavin Barraclough 2012-01-04 17:17:03 PST 
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.
Comment 3 Early Warning System Bot 2012-01-04 17:29:48 PST 
Comment on attachment 121188 [details]
Fix

Attachment 121188 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11115011
Comment 4 Gavin Barraclough 2012-01-05 00:02:20 PST 
Fixed in r104120.
Comment 5 Csaba Osztrogonác 2012-01-05 02:51:43 PST 
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
Comment 6 Csaba Osztrogonác 2012-01-05 03:33:32 PST 
Buildfix landed in http://trac.webkit.org/changeset/104136