Bug 148353 – [JSC] Reduce the memory usage of BytecodeLivenessAnalysis

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] Reduce the memory usage of BytecodeLivenessAnalysis

[Benjamin Poulain]

Created attachment 259709 [details]
Patch

[Darin Adler]

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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

Committed r188849: <http://trac.webkit.org/changeset/188849>