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 Framework | Assignee: | 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
Andreas Kling
2012-09-20 15:58:13 PDT
Created attachment 198012 [details]
Patch
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 on attachment 198012 [details] Patch Attachment 198012 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/154203 Created attachment 198015 [details]
Patch for landing
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 GCC is crashing on the EFL EWS after this patch. @Gyuyoung: What do? (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. (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. Created attachment 199032 [details]
Try to make EFL EWS happy
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 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. (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. Created attachment 199039 [details]
Minor CMake style fix
(In reply to comment #14) > Created an attachment (id=199039) [details] Thanks Raphael! Comment on attachment 199039 [details] Minor CMake style fix Clearing flags on attachment: 199039 Committed r148891: <http://trac.webkit.org/changeset/148891> All reviewed patches have been landed. Closing bug. (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 (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 (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 |