Bug 91303 - Rationalize and optimize storage allocation
Summary: Rationalize and optimize storage allocation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-13 18:01 PDT by Filip Pizlo
Modified: 2012-07-16 11:30 PDT (History)
8 users (show)

See Also:


Attachments
work in progress (20.48 KB, patch)
2012-07-13 18:03 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
Backwards Garen (22.17 KB, patch)
2012-07-14 14:00 PDT, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Backwards Frampton (25.27 KB, patch)
2012-07-14 14:52 PDT, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
almost there (28.28 KB, patch)
2012-07-14 16:56 PDT, Filip Pizlo
gns: commit-queue-
Details | Formatted Diff | Diff
the patch (30.19 KB, patch)
2012-07-14 17:14 PDT, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 )