Bug 179528

Summary: The memory consumption of DFG::BasicBlock can be easily reduced a bit
Product: WebKit Reporter: Robin Morisset <rmorisset>
Component: JavaScriptCoreAssignee: Robin Morisset <rmorisset>
Status: RESOLVED FIXED    
Severity: Minor CC: buildbot, commit-queue, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description Robin Morisset 2017-11-10 07:43:06 PST
I see several ways of doing it:
- Reordering a few fields to reduce padding brings sizeof(BasicBlock) from 3032 to 3016.
- Making Operands a single vector (safe since we never change the number of arguments) instead of 2 reduces the size from 3016 to 2984, and halves the number of memory allocations when both inline buffer overflow, and avoids overflowing in some cases when we have just a bit more locals than 16, but don't have the full 8 arguments, or the reverse. As a bonus, it makes BasicBlocks more readable when printed in the debugger.

This patch includes both of these changes (since the first one is fairly trivial).
No performance change on some quick benchmarks (none was expected, since this just reduces memory pressure a bit)

Another idea (for a possible further patch) would be to merge all five Operands in BasicBlock into a single Operand of a struct with 5 fields (since all Operands share the same number of locals and of arguments). This would reduce memory consumption and allocation more significantly, but would have a risk of harming memory locality depending on the access pattern. It would also be significantly more involved, since it would require fixing every location that uses one of these 5 fields.
Comment 1 Robin Morisset 2017-11-10 07:53:39 PST
Created attachment 326581 [details]
Patch
Comment 2 Build Bot 2017-11-10 07:55:08 PST
Attachment 326581 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGBasicBlock.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGBasicBlock.cpp:40:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGBasicBlock.cpp:41:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGBasicBlock.cpp:46:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGBasicBlock.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGBasicBlock.cpp:48:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Saam Barati 2017-11-10 08:24:19 PST
Comment on attachment 326581 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326581&action=review

r=me

> Source/JavaScriptCore/bytecode/Operands.h:186
>      {

You can just write this as:
operand(operand) = value

> Source/JavaScriptCore/bytecode/Operands.h:252
> +    size_t m_numArguments;

I say make this unsigned. This probably won’t matter because of padding, but it might be useful if someone adds a field in the future.
Comment 4 Robin Morisset 2017-11-10 08:36:48 PST
Created attachment 326589 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2017-11-10 08:56:23 PST
Comment on attachment 326589 [details]
Patch for landing

Rejecting attachment 326589 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
es/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource28.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource9.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource9.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/5177748
Comment 6 Robin Morisset 2017-11-10 09:08:34 PST
Created attachment 326592 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-11-10 09:40:03 PST
Comment on attachment 326592 [details]
Patch for landing

Clearing flags on attachment: 326592

Committed r224689: <https://trac.webkit.org/changeset/224689>
Comment 8 WebKit Commit Bot 2017-11-10 09:40:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-11-15 09:37:24 PST
<rdar://problem/35562082>