[Webkit-unassigned] [Bug 41920] Avoid slow-path for put() in Array.splice()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41920


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #61299|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #14 from Darin Adler <darin at apple.com>  2010-07-13 09:27:13 PST ---
(From update of attachment 61299)
This is really great work. And so close, but still one thing not quite right that I spotted.

> +    if (creationMode == CreateCompact) {
> +        // NOTE: setLength() must be called after initializing this array.
> +        m_storage->m_length = 0;
> +        m_storage->m_numValuesInVector = initialCapacity;

The intent here is to now call uncheckedSetIndex on each element in the vector and then call setLength. The comment mentions setLength, but not uncheckedSetIndex; both are required. I think the comment should go in the header instead of here (see below).

Unfortunately, the call to setLength when the values are set but the length is still zero is incompatible with CHECK_ARRAY_CONSISTENCY. The checkConsistency call at the start of setLength will assert because there are values in the vector in slots that are greater than m_length.

So somehow we need to turn off that consistency check. I'm sure there are multiple ways to do this. The simplest would be to remove the consistency check at the start of setLength, but I'd prefer not to do that. The second simplest I can think of is to add a debug-only boolean flag to ArrayStorage that is true if we have created an array with CreateCompact but have not yet set the length. If that flag is set we can skip the checkConsistency call at the start of setLength. We can also assert that flag is not set in the checkConsistency function. We could probably do other checks with that flag as well, to catch other unsafe actions during the process of creating the array.

> +    enum ArrayCreationMode { CreateCompact, CreateInitialized };

I think a comment about how CreateCompact works should be somewhere in the header. If we use CreateCompact to create an array, we are obligated to then use uncheckedSetIndex for all the elements of the array and the setLength; in fact the debugging checks may obligate us to do this even if the length is zero! The header should mentions this somewhere. Not that I want to be wordy and untidy, but that would be non-obvious if not mentioned, and so is the type of thing we do want to put in a comment.

> +        void uncheckedSetIndex(unsigned i, JSValue v)
> +        {
> +            ASSERT(canSetIndex(i));
> +            m_storage->m_vector[i] = v;
> +        }

We could also assert that we are in the "create new elements of a compact array" mode if we add that flag, or if not in that special mode we'd want to assert that the previous value in the vector is non-zero and that i < m_storage->m_length.

I'd say review- but I don't want to land something that breaks the CHECK_ARRAY_CONSISTENCY mode. I also suggest you run the regression tests with CHECK_ARRAY_CONSISTENCY turned on after making your change, although you don't want to land with it on!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.