Bug 97268 – Shrink baseline size of WTF::Vector on 64-bit by switching to unsigned capacity and size.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Andreas Kling]

I believe that using 32-bit indices for Vector addresses should be enough for WebKit, and if we switch from 'size_t' to 'unsigned', we can save 8 bytes per Vector on 64-bit platforms.
There are plenty of Vectors in both JSC and WebCore, and we can save quite a lot here.

[Radar WebKit Bug Importer]

<rdar://problem/12376519>

[Andreas Kling]

Created attachment 198012 [details]
Patch

[Benjamin Poulain]

Comment on attachment 198012 [details]
Patch

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

> Source/WTF/ChangeLog:14
> +
> +        Shrink Vector by 8 bytes on 64-bit by using 32-bit capacity and size.
> +        Vector now inherits from VectorBuffer instead of having a VectorBuffer member;
> +        this is necessary for m_size to fall into the padding after the base class members.
> +
> +        The WTF::Vector API still uses size_t.
> +

That is great!

Can we get some data on the outcome? (Binary size, dynamic size, etc).

[EFL EWS Bot]

Comment on attachment 198012 [details]
Patch

Attachment 198012 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/154203

[Andreas Kling]

Created attachment 198015 [details]
Patch for landing

[EFL EWS Bot]

Comment on attachment 198015 [details]
Patch for landing

Attachment 198015 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/74229

[Andreas Kling]

GCC is crashing on the EFL EWS after this patch.
@Gyuyoung: What do?

[Gyuyoung Kim]

(In reply to comment #7)
> GCC is crashing on the EFL EWS after this patch.
> @Gyuyoung: What do?

Something is weird. There is no problem when I build WebKit efl with this patch locally. I will check this on buildbot again.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> GCC is crashing on the EFL EWS after this patch.
> @Gyuyoung: What do?

Looks like GCC 4.6 with -O3 and that code makes the world explode. We're exploring some options in bug 114627.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 199032 [details]
Try to make EFL EWS happy

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/SizeLimits.cpp', u'Source/WTF/wtf/Vector.h', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog']" exit_code: 1
Source/WebCore/CMakeLists.txt:2567:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
Total errors found: 1 in 7 files


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

[Darin Adler]

Comment on attachment 199032 [details]
Try to make EFL EWS happy

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

We don’t need Vector<char> that are >4GB?

> Source/WTF/ChangeLog:13
> +        The WTF::Vector API still uses size_t.

Long term, that may be a mistake.

[Andreas Kling]

(In reply to comment #12)
> (From update of attachment 199032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199032&action=review
> 
> We don’t need Vector<char> that are >4GB?

Right now, no. If we need such huge vectors in the future, I suspect we'll want to look at something specialized.

> 
> > Source/WTF/ChangeLog:13
> > +        The WTF::Vector API still uses size_t.
> 
> Long term, that may be a mistake.

Yes, but doing that now would turn this into a monster patch. I think it'd be better to move the key clients to unsigned indexing one by one, and then change Vector when it's safe. WTF::notFound is a bit of a problem here, since both Vector and String APIs use the same size_t(-1) constant for that.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 199039 [details]
Minor CMake style fix

[Andreas Kling]

(In reply to comment #14)
> Created an attachment (id=199039) [details]

Thanks Raphael!

[Andreas Kling]

Comment on attachment 199039 [details]
Minor CMake style fix

Clearing flags on attachment: 199039

Committed r148891: <http://trac.webkit.org/changeset/148891>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

(In reply to comment #17)
> All reviewed patches have been landed.  Closing bug.

I think this might have caused some javascript core tests to fail:

2013-04-22 10:20:53.121 testapi[31838:507] Testing Objective-C API
ASSERTION FAILED: bitwise_cast<size_t*>(&testVector)[0] == 42
build/OpenSource/Source/JavaScriptCore/llint/LLIntData.cpp(119) : static void JSC::LLInt::Data::performAssertions(JSC::VM &)
1   0x10f5d075c JSC::LLInt::Data::performAssertions(JSC::VM&)
2   0x10f51c041 JSC::VM::VM(JSC::VM::VMType, JSC::HeapType)
3   0x10f51a821 JSC::VM::VM(JSC::VM::VMType, JSC::HeapType)
4   0x10f51cbbc JSC::VM::createContextGroup(JSC::HeapType)
5   0x10f51595b JSContextGroupCreate
6   0x10f58cae5 -[JSVirtualMachine init]
7   0x10f5144f9 -[JSContext init]
8   0x10f1fde44 testObjectiveCAPI
9   0x10f1f517c main
10  0x7fff89e5d7bd start
11  0x1

[Jessie Berlin]

(In reply to comment #18)
> (In reply to comment #17)
> > All reviewed patches have been landed.  Closing bug.
> 
> I think this might have caused some javascript core tests to fail:
> 
> 2013-04-22 10:20:53.121 testapi[31838:507] Testing Objective-C API
> ASSERTION FAILED: bitwise_cast<size_t*>(&testVector)[0] == 42
> build/OpenSource/Source/JavaScriptCore/llint/LLIntData.cpp(119) : static void JSC::LLInt::Data::performAssertions(JSC::VM &)
> 1   0x10f5d075c JSC::LLInt::Data::performAssertions(JSC::VM&)
> 2   0x10f51c041 JSC::VM::VM(JSC::VM::VMType, JSC::HeapType)
> 3   0x10f51a821 JSC::VM::VM(JSC::VM::VMType, JSC::HeapType)
> 4   0x10f51cbbc JSC::VM::createContextGroup(JSC::HeapType)
> 5   0x10f51595b JSContextGroupCreate
> 6   0x10f58cae5 -[JSVirtualMachine init]
> 7   0x10f5144f9 -[JSContext init]
> 8   0x10f1fde44 testObjectiveCAPI
> 9   0x10f1f517c main
> 10  0x7fff89e5d7bd start
> 11  0x1

Looks like Oliver fixed this in http://trac.webkit.org/changeset/148896

[Csaba Osztrogonác]

(In reply to comment #10)
> Created an attachment (id=199032) [details]
> Try to make EFL EWS happy

This cmake change was incorrect, fixes landed in 
- http://trac.webkit.org/changeset/150940
- http://trac.webkit.org/changeset/151083