Bug 91303

Summary:

Rationalize and optimize storage allocation

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fpizlo, ggaren, gustavo, hausmann, mhahnenberg, webkit.review.bot, xan.lopez, zherczeg

Priority:

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+

Filip Pizlo

Reported 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.

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 attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2012-07-13 18:03:53 PDT

Created attachment 152391 [details] work in progress

Filip Pizlo

Comment 2 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.

WebKit Review Bot

Comment 3 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.

Early Warning System Bot

Comment 4 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

Early Warning System Bot

Comment 5 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

Filip Pizlo

Comment 6 2012-07-14 14:52:36 PDT

Created attachment 152434 [details] Backwards Frampton This appears to undo the Kraken slow-down.

WebKit Review Bot

Comment 7 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.

Gyuyoung Kim

Comment 8 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

Early Warning System Bot

Comment 9 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

Early Warning System Bot

Comment 10 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

Build Bot

Comment 11 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

Early Warning System Bot

Comment 12 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

Early Warning System Bot

Comment 13 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

Build Bot

Comment 14 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

Gyuyoung Kim

Comment 15 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

Filip Pizlo

Comment 16 2012-07-14 16:56:38 PDT

Created attachment 152436 [details] almost there Some more assembly hackage to do.

WebKit Review Bot

Comment 17 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.

Filip Pizlo

Comment 18 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)

Gustavo Noronha (kov)

Comment 19 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

Filip Pizlo

Comment 20 2012-07-14 17:14:39 PDT

Created attachment 152437 [details] the patch

WebKit Review Bot

Comment 21 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.

Filip Pizlo

Comment 22 2012-07-14 21:03:15 PDT

Landed in http://trac.webkit.org/changeset/122677

Simon Hausmann

Comment 23 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 )

Filip Pizlo

Comment 24 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 )

Note You need to log in before you can comment on or make changes to this bug.