Bug 91303

Summary: Rationalize and optimize storage allocation
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, gustavo, hausmann, mhahnenberg, webkit.review.bot, xan.lopez, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
work in progress
none
Backwards Garen
webkit-ews: commit-queue-
Backwards Frampton
webkit-ews: commit-queue-
almost there
gustavo: commit-queue-
the patch oliver: review+

Description Filip Pizlo 2012-07-13 18:01:16 PDT 
Currently our storage allocator requires multiple branches, and multiple temporary registers, to be sound.  The code is also duplicated in two places (one for mutator and one for collector).

The code should be unified, and simplified to use backwards bumping allocation, which only requires one branch and one register.
Comment 1 Filip Pizlo 2012-07-13 18:03:53 PDT 
Created attachment 152391 [details]
work in progress
Comment 2 Filip Pizlo 2012-07-14 14:00:06 PDT 
Created attachment 152433 [details]
Backwards Garen

This appears to work, but because it removes the reallocation fast case optimization, we end up losing some performance on Kraken.

I will try Backwards Frampton-Pizlo next.
Comment 3 WebKit Review Bot 2012-07-14 14:18:29 PDT 
Attachment 152433 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/assembler/X86Assembler.h:341:  addq_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Early Warning System Bot 2012-07-14 14:34:35 PDT 
Comment on attachment 152433 [details]
Backwards Garen

Attachment 152433 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13232659
Comment 5 Early Warning System Bot 2012-07-14 14:36:42 PDT 
Comment on attachment 152433 [details]
Backwards Garen

Attachment 152433 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13232660
Comment 6 Filip Pizlo 2012-07-14 14:52:36 PDT 
Created attachment 152434 [details]
Backwards Frampton

This appears to undo the Kraken slow-down.
Comment 7 WebKit Review Bot 2012-07-14 14:54:12 PDT 
Attachment 152434 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/assembler/X86Assembler.h:341:  addq_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:452:  negq_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Gyuyoung Kim 2012-07-14 15:03:27 PDT 
Comment on attachment 152433 [details]
Backwards Garen

Attachment 152433 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13248106
Comment 9 Early Warning System Bot 2012-07-14 15:06:21 PDT 
Comment on attachment 152434 [details]
Backwards Frampton

Attachment 152434 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13230617
Comment 10 Early Warning System Bot 2012-07-14 15:10:10 PDT 
Comment on attachment 152434 [details]
Backwards Frampton

Attachment 152434 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13230618
Comment 11 Build Bot 2012-07-14 15:14:31 PDT 
Comment on attachment 152434 [details]
Backwards Frampton

Attachment 152434 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13242531
Comment 12 Early Warning System Bot 2012-07-14 15:31:23 PDT 
Comment on attachment 152433 [details]
Backwards Garen

Attachment 152433 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13235544
Comment 13 Early Warning System Bot 2012-07-14 15:34:07 PDT 
Comment on attachment 152433 [details]
Backwards Garen

Attachment 152433 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13232665
Comment 14 Build Bot 2012-07-14 15:36:50 PDT 
Comment on attachment 152433 [details]
Backwards Garen

Attachment 152433 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13233522
Comment 15 Gyuyoung Kim 2012-07-14 16:00:44 PDT 
Comment on attachment 152433 [details]
Backwards Garen

Attachment 152433 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13239520
Comment 16 Filip Pizlo 2012-07-14 16:56:38 PDT 
Created attachment 152436 [details]
almost there

Some more assembly hackage to do.
Comment 17 WebKit Review Bot 2012-07-14 16:59:28 PDT 
Attachment 152436 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/assembler/X86Assembler.h:308:  addl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:346:  addq_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:457:  negq_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Filip Pizlo 2012-07-14 16:59:55 PDT 
Zoltan, this will need some new opcodes, like:

add32(Address src, RegisterID dest)
add32(AbsoluteAddress src, RegisterID dest)
Comment 19 Gustavo Noronha (kov) 2012-07-14 17:05:57 PDT 
Comment on attachment 152436 [details]
almost there

Attachment 152436 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13235562
Comment 20 Filip Pizlo 2012-07-14 17:14:39 PDT 
Created attachment 152437 [details]
the patch
Comment 21 WebKit Review Bot 2012-07-14 17:16:59 PDT 
Attachment 152437 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/assembler/X86Assembler.h:309:  addl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:348:  addq_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:459:  negq_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Filip Pizlo 2012-07-14 21:03:15 PDT 
Landed in http://trac.webkit.org/changeset/122677
Comment 23 Simon Hausmann 2012-07-16 05:47:40 PDT 
Looks like this broke the Qt ARMv7 build:


In file included from JavaScriptCore/jit/JITWriteBarrier.h:31,
                 from JavaScriptCore/bytecode/CallLinkInfo.h:30,
                 from JavaScriptCore/bytecode/CodeBlock.h:34,
                 from JavaScriptCore/API/JSCallbackFunction.cpp:31:
.../JavaScriptCore/assembler/MacroAssembler.h: In member function 'void JSC::MacroAssembler::addPtr(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::AbsoluteAddress, JSC::ARMRegisters::RegisterID)':
.../JavaScriptCore/assembler/MacroAssembler.h:290: error: no matching function for call to 'JSC::MacroAssembler::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::AbsoluteAddress&, JSC::ARMRegisters::RegisterID&)'
.../JavaScriptCore/assembler/MacroAssemblerARM.h:88: note: candidates are: void JSC::MacroAssemblerARM::add32(JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID)
.../JavaScriptCore/assembler/MacroAssemblerARM.h:93: note:                 void JSC::MacroAssemblerARM::add32(JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID)
.../JavaScriptCore/assembler/MacroAssemblerARM.h:98: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::Address)
.../JavaScriptCore/assembler/MacroAssemblerARM.h:105: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::ARMRegisters::RegisterID)
.../JavaScriptCore/assembler/MacroAssemblerARM.h:110: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::Address, JSC::ARMRegisters::RegisterID)
.../JavaScriptCore/assembler/MacroAssemblerARM.h:116: note:                 void JSC::MacroAssemblerARM::add32(JSC::ARMRegisters::RegisterID, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::ARMRegisters::RegisterID)
.../Source/JavaScriptCore/assembler/MacroAssemblerARM.h:837: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID)
.../JavaScriptCore/assembler/MacroAssemblerARM.h:842: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::AbsoluteAddress)

( from http://build.webkit.org/builders/Qt%20Linux%20ARMv7%20Release/builds/48787 )
Comment 24 Filip Pizlo 2012-07-16 11:30:28 PDT 
Yes, because this patch requires new assembler functionality.  See Comment #18.

(In reply to comment #23)
> Looks like this broke the Qt ARMv7 build:
> 
> 
> In file included from JavaScriptCore/jit/JITWriteBarrier.h:31,
>                  from JavaScriptCore/bytecode/CallLinkInfo.h:30,
>                  from JavaScriptCore/bytecode/CodeBlock.h:34,
>                  from JavaScriptCore/API/JSCallbackFunction.cpp:31:
> .../JavaScriptCore/assembler/MacroAssembler.h: In member function 'void JSC::MacroAssembler::addPtr(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::AbsoluteAddress, JSC::ARMRegisters::RegisterID)':
> .../JavaScriptCore/assembler/MacroAssembler.h:290: error: no matching function for call to 'JSC::MacroAssembler::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::AbsoluteAddress&, JSC::ARMRegisters::RegisterID&)'
> .../JavaScriptCore/assembler/MacroAssemblerARM.h:88: note: candidates are: void JSC::MacroAssemblerARM::add32(JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID)
> .../JavaScriptCore/assembler/MacroAssemblerARM.h:93: note:                 void JSC::MacroAssemblerARM::add32(JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID)
> .../JavaScriptCore/assembler/MacroAssemblerARM.h:98: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::Address)
> .../JavaScriptCore/assembler/MacroAssemblerARM.h:105: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::ARMRegisters::RegisterID)
> .../JavaScriptCore/assembler/MacroAssemblerARM.h:110: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::Address, JSC::ARMRegisters::RegisterID)
> .../JavaScriptCore/assembler/MacroAssemblerARM.h:116: note:                 void JSC::MacroAssemblerARM::add32(JSC::ARMRegisters::RegisterID, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::ARMRegisters::RegisterID)
> .../Source/JavaScriptCore/assembler/MacroAssemblerARM.h:837: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::ARMRegisters::RegisterID, JSC::ARMRegisters::RegisterID)
> .../JavaScriptCore/assembler/MacroAssemblerARM.h:842: note:                 void JSC::MacroAssemblerARM::add32(JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::AbsoluteAddress)
> 
> ( from http://build.webkit.org/builders/Qt%20Linux%20ARMv7%20Release/builds/48787 )