Bug 97268

Summary: Shrink baseline size of WTF::Vector on 64-bit by switching to unsigned capacity and size.
Product: WebKit Reporter: Andreas Kling <kling>
Component: Web Template FrameworkAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, commit-queue, d-r, gyuyoung.kim, jberlin, kling, ossy, rakuco, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 114627    
Bug Blocks:    
Attachments:
Description Flags
Patch
sam: review+, eflews.bot: commit-queue-
Patch for landing
none
Try to make EFL EWS happy
none
Minor CMake style fix none

Description Andreas Kling 2012-09-20 15:58:13 PDT
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.
Comment 1 Radar WebKit Bug Importer 2012-09-26 07:48:11 PDT
<rdar://problem/12376519>
Comment 2 Andreas Kling 2013-04-14 15:07:46 PDT
Created attachment 198012 [details]
Patch
Comment 3 Benjamin Poulain 2013-04-14 15:25:23 PDT
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).
Comment 4 EFL EWS Bot 2013-04-14 15:30:00 PDT
Comment on attachment 198012 [details]
Patch

Attachment 198012 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/154203
Comment 5 Andreas Kling 2013-04-14 17:06:14 PDT
Created attachment 198015 [details]
Patch for landing
Comment 6 EFL EWS Bot 2013-04-14 17:19:38 PDT
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
Comment 7 Andreas Kling 2013-04-14 17:22:25 PDT
GCC is crashing on the EFL EWS after this patch.
@Gyuyoung: What do?
Comment 8 Gyuyoung Kim 2013-04-14 18:06:14 PDT
(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.
Comment 9 Raphael Kubo da Costa (:rakuco) 2013-04-19 06:18:04 PDT
(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.
Comment 10 Raphael Kubo da Costa (:rakuco) 2013-04-22 08:19:39 PDT
Created attachment 199032 [details]
Try to make EFL EWS happy
Comment 11 WebKit Commit Bot 2013-04-22 08:22:00 PDT
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.
Comment 12 Darin Adler 2013-04-22 08:41:11 PDT
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.
Comment 13 Andreas Kling 2013-04-22 08:45:37 PDT
(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.
Comment 14 Raphael Kubo da Costa (:rakuco) 2013-04-22 09:36:55 PDT
Created attachment 199039 [details]
Minor CMake style fix
Comment 15 Andreas Kling 2013-04-22 09:43:20 PDT
(In reply to comment #14)
> Created an attachment (id=199039) [details]

Thanks Raphael!
Comment 16 Andreas Kling 2013-04-22 09:59:14 PDT
Comment on attachment 199039 [details]
Minor CMake style fix

Clearing flags on attachment: 199039

Committed r148891: <http://trac.webkit.org/changeset/148891>
Comment 17 Andreas Kling 2013-04-22 09:59:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Jessie Berlin 2013-04-22 11:12:35 PDT
(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
Comment 19 Jessie Berlin 2013-04-22 11:44:30 PDT
(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
Comment 20 Csaba Osztrogonác 2013-06-02 13:03:21 PDT
(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