[JSC] Reduce the memory usage of BytecodeLivenessAnalysis
Created attachment 259709 [details] Patch
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.
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.
Committed r188849: <http://trac.webkit.org/changeset/188849>