Bug 72198 – [V8][Chromium]Serialize dense arrays densly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dmitry Lomov]

SerializeScriptValue should:
1. Serialize dense array densely
2. Ensure that on deserialization, sparse arrays are allocated as sparse.

[Dmitry Lomov]

Created attachment 114808 [details]
Fix

After this patch, Microsoft Web Fountains demo (http://ie.microsoft.com/testdrive/Graphics/WorkerFountains/Default.html) no longer suffers FPS hit when webworkers are enabled.

[WebKit Review Bot]

Comment on attachment 114808 [details]
Fix

Attachment 114808 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10346749

New failing tests:
fast/dom/Window/window-postmessage-clone-deep-array.html
fast/dom/Window/window-postmessage-clone.html

[David Levin]

Comment on attachment 114808 [details]
Fix

Due to new failing tests and (the apparent) the lack of backward compatibility. I'm pretty sure that these formats may be saved so we need to be able to deserialize them.

[Dmitry Lomov]

(In reply to comment #3)
> (From update of attachment 114808 [details])
> Due to new failing tests and (the apparent) the lack of backward compatibility. I'm pretty sure that these formats may be saved so we need to be able to deserialize them.

Tests obviously shouldn't fail, so I'll have a look.
However, this change is backward-compatible (all values serialized by the version of the algorithm without the patch will be correctly deserialzied by the algorithm with the patch)

[David Levin]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 114808 [details] [details])
> > Due to new failing tests and (the apparent) the lack of backward compatibility. I'm pretty sure that these formats may be saved so we need to be able to deserialize them.
> 
> Tests obviously shouldn't fail, so I'll have a look.
> However, this change is backward-compatible (all values serialized by the version of the algorithm without the patch will be correctly deserialzied by the algorithm with the patch)

How can that be when "ArrayTag = '['" got deleted?

[Dmitry Lomov]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 114808 [details] [details] [details])
> > > Due to new failing tests and (the apparent) the lack of backward compatibility. I'm pretty sure that these formats may be saved so we need to be able to deserialize them.
> > 
> > Tests obviously shouldn't fail, so I'll have a look.
> > However, this change is backward-compatible (all values serialized by the version of the algorithm without the patch will be correctly deserialzied by the algorithm with the patch)
> 
> How can that be when "ArrayTag = '['" got deleted?

ArrayTag has never been written to the stream (you can see that ArrayState was under #if 0 ... #endif, now removed)

[Dmitry Lomov]

Created attachment 115073 [details]
Tests fixed

Two issues caught by the tests:
1. DenseArrayState::objectDone now writes the serialized length, not the length of an array after serialization (the latter may change due to accessor execution)
2. Removed extra state push around the call to startArrayState

[David Levin]

Comment on attachment 115073 [details]
Tests fixed

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

There are a few things here that I think it would be good to address. Thanks!

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:197
> +                         //                                             fills it with the last lenght elements and numProperties name,value pairs pushed onto deserialization stack

spacing off by one on this second line.

 "lenght" should be "length"

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:749
> +                    if (hasStringProperty || (hasIndexedProperty && !ignoreIndexed))

Why is this change done? What is being fixed?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:840
> +            return serializeProperties(true, serializer);

It looks like this writes out indexes followed by properties but the deserializer expects things in the reverse order. (Please add a test.)

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:847
> +        }

add blank line.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:849
> +        uint32_t m_arrayIndex;

Why is m_arrayIndex a member variable?

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1024
> +    static bool serializeDensely(uint32_t length, uint32_t propertyCount) 

The name sounds ambiguous right now. One could easily interpret the name to mean that it will do the serialization. Consider "shouldSerializeDensely".

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1029
> +        // so densly is better than sparsly whenever 6*propertyCount > length

densely

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1281
> +       case RegExpTag:

spacing off.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1355
> +            if (!creator.newSparseArray(length))

I find this confusing.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1848
> +        if (m_version > 0) {

Why is there a version check here when this is a new field?

(I guess I expect a failure if the version is below a certain number.)

Also should we be increasing the version?

[Dmitry Lomov]

Comment on attachment 115073 [details]
Tests fixed

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

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:197
>> +                         //                                             fills it with the last lenght elements and numProperties name,value pairs pushed onto deserialization stack
> 
> spacing off by one on this second line.
> 
>  "lenght" should be "length"

Done

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:749
>> +                    if (hasStringProperty || (hasIndexedProperty && !ignoreIndexed))
> 
> Why is this change done? What is being fixed?

DenseArrayState serializes indexed properties separately.

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:840
>> +            return serializeProperties(true, serializer);
> 
> It looks like this writes out indexes followed by properties but the deserializer expects things in the reverse order. (Please add a test.)

There is a test for this, so it is all good :)
I think you are confused because deserializer picks elements from the stack, where they are in reverse order

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:847
>> +        }
> 
> add blank line.

Done

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:849
>> +        uint32_t m_arrayIndex;
> 
> Why is m_arrayIndex a member variable?

Advance can be called multiple times, if serialization of array elements push extra states to the stack

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1024
>> +    static bool serializeDensely(uint32_t length, uint32_t propertyCount) 
> 
> The name sounds ambiguous right now. One could easily interpret the name to mean that it will do the serialization. Consider "shouldSerializeDensely".

Done.

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1029
>> +        // so densly is better than sparsly whenever 6*propertyCount > length
> 
> densely

Done

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1281
>> +       case RegExpTag:
> 
> spacing off.

Done.

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1355
>> +            if (!creator.newSparseArray(length))
> 
> I find this confusing.

Good catch! Fixed.

>> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1848
>> +        if (m_version > 0) {
> 
> Why is there a version check here when this is a new field?
> 
> (I guess I expect a failure if the version is below a certain number.)
> 
> Also should we be increasing the version?

Yeah version check is dead code - removed.
We shouldn't increase version - this is a backward compatible change.

[Dmitry Lomov]

Created attachment 115083 [details]
CR comments addressed

[WebKit Review Bot]

Comment on attachment 115083 [details]
CR comments addressed

Clearing flags on attachment: 115083

Committed r100239: <http://trac.webkit.org/changeset/100239>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

The new test fails on JSC platforms, please check https://bugs.webkit.org/show_bug.cgi?id=72363

[Simon Fraser (smfr)]

This seems to have caused
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100241%20(34700)/fast/dom/Window/window-postmessage-arrays-pretty-diff.html

[Dmitry Lomov]

(In reply to comment #14)
> This seems to have caused
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100241%20(34700)/fast/dom/Window/window-postmessage-arrays-pretty-diff.html

(In reply to comment #14)
> This seems to have caused
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100241%20(34700)/fast/dom/Window/window-postmessage-arrays-pretty-diff.html


I have opened https://bugs.webkit.org/show_bug.cgi?id=72435.
What's the best way to address failing tests until that is fixed?