Bug 54524 – Allow JSObject to fully utilize cell's capacity for inline storage.

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

[Gavin Barraclough]

Currently JSObject is both directly instantiated for regular JS objects, and derived to implement subtypes.  A consequence of this is that we need to ensure that sufficient space from the cell is left unused and available for any data members that will be introduced by subclasses of JSObject.  By restructuring the internal storage array out of JSObject we can increase the size in the internal storage for regular objects.

[Gavin Barraclough]

Created attachment 82592 [details]
The patch, this shows a small sunspider regression, but a similar v8 progression - a wash overall.

[WebKit Review Bot]

Attachment 82592 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSObjectRef.cpp'..." exit_code: 1

Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSObject.h:307:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/runtime/JSObject.h:337:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/runtime/JSNotAnObject.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/ObjectPrototype.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSByteArray.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/ErrorInstance.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 7 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 82592 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7926085

[Geoffrey Garen]

Looks like you need to update some platform-specific files to fix the Qt build:

../../../Source/WebCore/bridge/qt/qt_runtime.cpp: In member function ‘void JSC::Bindings::QtConnectionObject::execute(void**)’:
../../../Source/WebCore/bridge/qt/qt_runtime.cpp:1835: error: ‘createStructure’ is not a member of ‘JSC::JSObject’

[Geoffrey Garen]

Comment on attachment 82592 [details]
The patch, this shows a small sunspider regression, but a similar v8 progression - a wash overall.

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

r=me if you fix the build.

> Source/JavaScriptCore/ChangeLog:17
> +        and only allows construction through JSFinalObject::create().

I think you should mention the reason for this change -- now all objects have a direct pointer to their storage, and lea shenanigans are not required.

> Source/JavaScriptCore/runtime/JSObject.h:298
> +COMPILE_ASSERT((JSFinalObject_inlineStorageCapacity >= JSNonFinalObject_inlineStorageCapacity), vanilla_storage_is_at_least_as_large_as_non_vanilla);

s/vanilla/final/

> Source/JavaScriptCore/runtime/JSTypeInfo.h:73
> +        unsigned isVanilla() const { return m_flags2 && (IsJSFinalObject >> 8); }

s/Vanilla/Final/

[Gavin Barraclough]

Created attachment 82682 [details]
Fixes for review comments / Qt (not for style though - these all look like preexisting issues).

[Geoffrey Garen]

Comment on attachment 82682 [details]
Fixes for review comments / Qt (not for style though - these all look like preexisting issues).

I believe the EWS bots will keep running even after an r+, so I'll mark this r+ now.

[WebKit Review Bot]

Attachment 82682 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSObjectRef.cpp'..." exit_code: 1

Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSObject.h:312:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/runtime/JSObject.h:344:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/runtime/JSNotAnObject.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/ObjectPrototype.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSByteArray.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/ErrorInstance.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 7 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

fixed in r78732.