Summary: | Unnecessary operations, duplicated codes in the CodeGenerator | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Judit Jász <jasy> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||||
Severity: | Normal | CC: | barraclough, ggaren, oliver, zwarich | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Judit Jász
2008-10-15 07:19:47 PDT
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.
(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? Created attachment 24452 [details]
performance result
The attached file contains the result of the patch on performance.
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. 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.
Created attachment 24747 [details]
performance results of the modified patch
I'll test on Mac and review when I get to work. 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.
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.
Does this bug still track an actionable problem? 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. |