91303 – Rationalize and optimize storage allocation

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

Importance:	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)
CC List:	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	gustavo: 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
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

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 )