Bug 148353

Summary: [JSC] Reduce the memory usage of BytecodeLivenessAnalysis
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Benjamin Poulain 2015-08-21 20:57:16 PDT
[JSC] Reduce the memory usage of BytecodeLivenessAnalysis
Comment 1 Benjamin Poulain 2015-08-21 21:08:05 PDT
Created attachment 259709 [details]
Patch
Comment 2 Darin Adler 2015-08-23 09:31:15 PDT
Comment on attachment 259709 [details]
Patch

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

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:113
> +void computeBytecodeBasicBlocks(CodeBlock* codeBlock, Vector<std::unique_ptr<BytecodeBasicBlock> >& basicBlocks)

No need for the space between the ">" characters.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:122
> +    std::unique_ptr<BytecodeBasicBlock> entry(new BytecodeBasicBlock(BytecodeBasicBlock::EntryBlock));
> +    std::unique_ptr<BytecodeBasicBlock> firstBlock(new BytecodeBasicBlock(0, 0));

WebKit coding style is to make_unique so there’s no separate “new”:

   auto entry = std::make_unique<BytecodeBasicBlock>(BytecodeBasicBlock::EntryBlock);
   auto firstBlock = std::make_unique<BytecodeBasicBlock>(0, 0);

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:143
> +            std::unique_ptr<BytecodeBasicBlock> newBlock(new BytecodeBasicBlock(bytecodeOffset, opcodeLength));

auto newBlock = std::make_unique<BytecodeBasicBlock>(bytecodeOffset, opcodeLength);

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:40
> +class BytecodeBasicBlock {
> +    WTF_MAKE_FAST_ALLOCATED;

Since there’s no polymorphism involved, why are we allocating these on the heap at all? Can we just use Vector<BytecodeBasicBlock> instead? I suppose the problem there is that the blocks will be moved so we can’t use pointers in m_successors?

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:73
> +void computeBytecodeBasicBlocks(CodeBlock*, Vector<std::unique_ptr<BytecodeBasicBlock> >&);

No need for the space between the > characters.
Comment 3 Benjamin Poulain 2015-08-23 22:52:50 PDT
Thanks for the review!

> WebKit coding style is to make_unique so there’s no separate “new”:

You are not making it easy on my dislike for auto :)

> > Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:40
> > +class BytecodeBasicBlock {
> > +    WTF_MAKE_FAST_ALLOCATED;
> 
> Since there’s no polymorphism involved, why are we allocating these on the
> heap at all? Can we just use Vector<BytecodeBasicBlock> instead? I suppose
> the problem there is that the blocks will be moved so we can’t use pointers
> in m_successors?

You are right, I explained that in the changelog. I wanted this patch to be simple and mechanical but this can definitely be improved further.
Comment 4 Benjamin Poulain 2015-08-23 23:18:52 PDT
Committed r188849: <http://trac.webkit.org/changeset/188849>