Bug 21611 – Unnecessary operations, duplicated codes in the CodeGenerator

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Judit Jász]

Since the CodeGenerator is one of the most important parts of the JavaScriptCode, it should be easily understandable and perspiciuous.
There are some code duplications, and sometimes there are unnecessary function calls or unnecessary computations especially where the generator calls one of the rewind functions. We can eliminate these by the refactoring of the code. With the eliminatino of the unnecessary computations we could achive a better performance, while  with the elimination of the duplicated codes the understanding of the code generation could be perspiciuous.

[Judit Jász]

Created attachment 24360 [details]
proposed patch

The patch has a measurable but not significant impact on the performance. 
Two new methods are introduced, which modify the value of the last instruction. In this way we do not have to remove the last instruction and after add a new one to the instruction chain if we only want to modify the last instruction. The patch contains other smaller code refactorings, which eliminates some duplicated codes.

[Mark Rowe (bdash)]

(In reply to comment #1)
> Created an attachment (id=24360) [edit]
> proposed patch
> 
> The patch has a measurable but not significant impact on the performance. 

What is the impact on performance?

[Judit Jász]

Created attachment 24452 [details]
performance result

The attached file contains the result of the patch on performance.

[Geoffrey Garen]

Comment on attachment 24360 [details]
proposed patch

Looks good to me. I've asked Cameron to review further, since he wrote most of the code being modified here.

Some simple style comments:

> +void ALWAYS_INLINE CodeGenerator::modifyLastBinaryOp(OpcodeID opcodeID, const int dst, const int src1, const int src2)
> +{
> +    ASSERT(instructions().size() >= 4);
> +    size_t size = instructions().size();
> +    instructions().at(size - 4).u.opcode = globalData()->machine->getOpcode(opcodeID);
> +    m_lastOpcodeID = opcodeID; 
> +    instructions().at(size - 3).u.operand = dst;
> +    instructions().at(size - 2).u.operand = src1;
> +    instructions().at(size - 1).u.operand = src2;
> +}

I would call this function "overwriteLastBinaryOp" instead of "modify", since "modify" is more abstract.

I would add an ASSERT: "ASSERT(globalData()->machine->isOpcode(instructions().at(size - 4).u.opcode))", to verify that the last instruction had the format you expected.

> +void ALWAYS_INLINE CodeGenerator::modifyLastUnaryOp(OpcodeID opcodeID, const int dst, const int src)
> +{
> +    ASSERT(instructions().size() >= 3);
> +    size_t size = instructions().size();
> +    instructions().at(size - 3).u.opcode = globalData()->machine->getOpcode(opcodeID);
> +    m_lastOpcodeID = opcodeID; 
> +    instructions().at(size - 2).u.operand = dst;
> +    instructions().at(size - 1).u.operand = src;
> +}

Same comments here.

> +        static int unknown() {
> +            return OperandTypes().toInt();
> +        }

> 695  RegisterID* CodeGenerator::emitBinaryOp(OpcodeID opcode, RegisterID* dst, RegisterID* src1, RegisterID* src2, OperandTypes types)
>  694 RegisterID* CodeGenerator::emitBinaryOp(OpcodeID opcode, RegisterID* dst, RegisterID* src1, RegisterID* src2, int types)

I don't understand why you had to make these changes.

Annotating your ChangeLog with explanations of what you changed and why would help explain this kind of thing to a reviewer, and a programmer looking back on this code later. It's also the WebKit policy to do so.

[Judit Jász]

Created attachment 24746 [details]
modified patch

I have modified my earlier patch. 
My aim with the refactoring of the CodeGenerator is to see how the implemented peepholes work now. Because they use the same steps almost everywhere, the code could be more understandable if we can reduce the number of duplicated code fragments. 

An other aim of this work is to eliminating the unnecessary steps of these peepholes. In these peepholes we change only the opcode of the last instructions and some of the operands. It is unnecessary to delete the last instruction and build a new one, it is suffucuent if we change the corresponding part of the last instruction.

[Judit Jász]

Created attachment 24747 [details]
performance results of the modified patch

[Cameron Zwarich (cpst)]

I'll test on Mac and review when I get to work.

[Cameron Zwarich (cpst)]

Created attachment 24748 [details]
Mac performance results

Here are the Mac performance results for your patch. Strangely, it seems to be a slowdown across the board.

[Cameron Zwarich (cpst)]

Comment on attachment 24746 [details]
modified patch

I cleared the review flag because of the performance issue on the Mac. I agree that having less code in the peephole optimizer is a better idea and I am a bit disappointed that this patch is a slowdown. I will try again or try to modify the patch to make it even.

[Alexey Proskuryakov]

Does this bug still track an actionable problem?

[Gavin Barraclough]

This patch will no longer apply, no response to Alexey's question from 2 years ago - closing, please reopen if you are still investigating this.