Bug 206986 – [JSC] Remove unnecessary allocations in BytecodeBasicBlock

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

[Yusuke Suzuki]

[JSC] Remove unnecessary allocations in BytecodeBasicBlock

[Yusuke Suzuki]

Created attachment 389229 [details]
Patch

[Mark Lam]

Comment on attachment 389229 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:38
> +inline BytecodeBasicBlock::BytecodeBasicBlock(const InstructionStream::Ref& instruction, unsigned index)

I recommend renaming index to blockIndex to make it clear that it's not the instruction index.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:46
> +inline BytecodeBasicBlock::BytecodeBasicBlock(BytecodeBasicBlock::SpecialBlockType blockType, unsigned index)

Ditto.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:53
> +    BytecodeBasicBlock(const InstructionStream::Ref&, unsigned);
> +    BytecodeBasicBlock(SpecialBlockType, unsigned);

I recommend naming the unsigned params as blockIndex to document their intended use.
I also recommend explicitly declaring these as inline.  That will help the compiler error out immediately if any clients try to use these, rather than waiting for the linker to find the error.

Alternatively, make these private and declare the Vector template a friend (is that possible?) since they are only intended to be constructed in a Vector.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:71
> +    explicit operator bool() const { return true; }

Why do we need this?

My understanding is that this is only needed by the Vector implementation, and that it should always return true because we'll never have an "empty" BytecodeBasicBlock in the Vectors that we use it in.  If this is correct, can you add a comment to document that?  I this logic is not necessarily obvious.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:82
> +    void addLength(unsigned length);

nit: the length param name seems obvious and therefore, is probably not needed.

> Source/JavaScriptCore/bytecode/Opcode.h:91
> +#define CALCULATE_MAX_LENGTH(id, length) length,

This appears to be unused.  Please remove.

> Source/JavaScriptCore/bytecode/Opcode.h:94
> +#undef CALCULATE_MAX_LENGTH

Ditto.  Please remove.

[Yusuke Suzuki]

Committed r255459: <https://trac.webkit.org/changeset/255459>

[Radar WebKit Bug Importer]

<rdar://problem/59042458>

[Yusuke Suzuki]

Comment on attachment 389229 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:38
>> +inline BytecodeBasicBlock::BytecodeBasicBlock(const InstructionStream::Ref& instruction, unsigned index)
> 
> I recommend renaming index to blockIndex to make it clear that it's not the instruction index.

Fixed.

>> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:46
>> +inline BytecodeBasicBlock::BytecodeBasicBlock(BytecodeBasicBlock::SpecialBlockType blockType, unsigned index)
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:53
>> +    BytecodeBasicBlock(SpecialBlockType, unsigned);
> 
> I recommend naming the unsigned params as blockIndex to document their intended use.
> I also recommend explicitly declaring these as inline.  That will help the compiler error out immediately if any clients try to use these, rather than waiting for the linker to find the error.
> 
> Alternatively, make these private and declare the Vector template a friend (is that possible?) since they are only intended to be constructed in a Vector.

putting inline looks better since it does not need to deal with weird Vector<> + friend class/function declaration. Fixed.

>> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:71
>> +    explicit operator bool() const { return true; }
> 
> Why do we need this?
> 
> My understanding is that this is only needed by the Vector implementation, and that it should always return true because we'll never have an "empty" BytecodeBasicBlock in the Vectors that we use it in.  If this is correct, can you add a comment to document that?  I this logic is not necessarily obvious.

This is required for IndexedContainerIterator::findNext.

>> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:82
>> +    void addLength(unsigned length);
> 
> nit: the length param name seems obvious and therefore, is probably not needed.

Removed.

>> Source/JavaScriptCore/bytecode/Opcode.h:91
>> +#define CALCULATE_MAX_LENGTH(id, length) length,
> 
> This appears to be unused.  Please remove.

Nice, removed.

>> Source/JavaScriptCore/bytecode/Opcode.h:94
>> +#undef CALCULATE_MAX_LENGTH
> 
> Ditto.  Please remove.

Removed.