[webkit-reviews] review denied: [Bug 41920] Avoid slow-path for put() in Array.splice() : [Attachment 61299] Proposed patch v4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 41920: Avoid slow-path for put() in Array.splice()
https://bugs.webkit.org/show_bug.cgi?id=41920

Attachment 61299: Proposed patch v4
https://bugs.webkit.org/attachment.cgi?id=61299&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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!