Summary: | JavaScriptCore should use simple type inferencing to speed-up AddNode | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||
Component: | JavaScriptCore | Assignee: | Eric Seidel (no email) <eric> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Bug Depends on: | 15879 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2007-11-07 07:28:15 PST
This is next on my list. Created attachment 17167 [details]
A fix (SunSpider claims this is a wash)
This speeds up some tests, and slows down others (not sure why!?). SunSpider claims this is an overall wash.
Created attachment 17168 [details]
Better fix (including LessNode), SunSpider claims this is 1.0% speedup
Created attachment 17170 [details]
Better fix (including LessNode), SunSpider claims this is > 0.5% speedup
Lots of noise on my system when running sunspider... I fixed one bug with this patch, it should be good to go now.
The re-write of the Bison %union was originally part of much larger parser fixes. In later patches I decided those other fixes were not necessary and removed them. The %union cleanup remained. If necessary, I can pull that part from this patch. I think the cleanup is good though. One could still add AddNodeNumberLeft and AddNodeNumberRight for a probably small speedup. These would avoid evaluateToNumber needing to fall back to evaluate for one of the values. Currently AddNode::evaluateToNumber needs to call evaluate for both values instead of evaluateToNumber (at least to avoid branching). Comment on attachment 17170 [details]
Better fix (including LessNode), SunSpider claims this is > 0.5% speedup
You have this expression in a few places:
const JSValue*&
If this is an out parameter of a JSValue*, it should be JSValue*&. The const doesn't make sense in this case.
I'm a little concerned about adding m_expectedReturnType to every Node -- isn't that going to make the node tree significantly bigger? If we are going to add it perhaps it could go in ExpressionNode rather than Node?
You'll also need to merge this with my evaluateToBoolean change.
+ NullNode() KJS_FAST_CALL { m_expectedReturnType = NullType; }
The base class constructor should take this type as a parameter. Otherwise, we're setting m_expectedReturnType twice, once in the base class constructor and once in the derived class.
Otherwise looks good.
(In reply to comment #7) > (From update of attachment 17170 [details] [edit]) > You have this expression in a few places: > > const JSValue*& > If this is an out parameter of a JSValue*, it should be JSValue*&. The const > doesn't make sense in this case. I agree. I did it to allow returning "this" from getPrimitiveNumber (which is const), but better would have been to just remove the const on getPrimitiveNumber. > I'm a little concerned about adding m_expectedReturnType to every Node -- isn't > that going to make the node tree significantly bigger? If we are going to add > it perhaps it could go in ExpressionNode rather than Node? I put it on node so it could be a bit field. I reduced the size of m_line by 3 bits, so it should be fine. > You'll also need to merge this with my evaluateToBoolean change. > > + NullNode() KJS_FAST_CALL { m_expectedReturnType = NullType; } > > The base class constructor should take this type as a parameter. Otherwise, > we're setting m_expectedReturnType twice, once in the base class constructor > and once in the derived class. Yeah, I had that initially. I can't remember why I didn't use it in the end. (In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 17170 [details] [edit] [edit]) > > You'll also need to merge this with my evaluateToBoolean change. > > > > + NullNode() KJS_FAST_CALL { m_expectedReturnType = NullType; } > > > > The base class constructor should take this type as a parameter. Otherwise, > > we're setting m_expectedReturnType twice, once in the base class constructor > > and once in the derived class. > > Yeah, I had that initially. I can't remember why I didn't use it in the end. Ah, so why I did this is that due to the fact that m_expectedReturnType is on Node instead of ExpressionNode (so it can be part of the bitfield and not bloat Node/ExpressionNode any). Node() would need to take an extra parameter which makes no sense for StatementNode. I could have a second protected constructor which took the return type... neither way is particularly pretty, but I guess the second protected constructor is "faster". Created attachment 17181 [details]
Patch updated to TOT and addressing Darin's concerns
Comment on attachment 17181 [details]
Patch updated to TOT and addressing Darin's concerns
269 __ZN3KJS8JSObject18getPrimitiveNumberEPNS_9ExecStateERdRPNS_7JSValueE
Why does this need to be exported? Who calls it outside JavaScriptCore?
925 // Both are objects (or unknown) -- fixme: this is kinda an inefficient way to get here.
I don't understand that comment, and the format seems strange.
1928 double n2 = term2->evaluateToNumber(exec);
1929 KJS_CHECKEXCEPTIONNUMBER
1930
1931 return n1 + n2;
I don't think you really need this exception check here since it's the last call to evaluate in another evaluate. The inner evaluate will already have attached the file and line number and there's no need to optimize out the addition.
1949 JSValue* v2 = term2->evaluate(exec);
1950 KJS_CHECKEXCEPTIONVALUE
Same here.
2134 KJS_CHECKEXCEPTIONVALUE
2135 return jsBoolean(n1 < n2);
And here.
2143 KJS_CHECKEXCEPTIONVALUE
2144 return jsBoolean(static_cast<StringImp*>(v1)->value() < static_cast<StringImp*>(v2)->value());
and here.
121 JSType expectedReturnType() const KJS_FAST_CALL { return m_expectedReturnType; }
Shouldn't this function move into ExpressionNode? I know that currently every Node has m_expectedReturnType, but it would be nice not to make the promise for the future.
160 JSType m_expectedReturnType : 3;
I think to make this work properly on Windows this needs to be unsigned, not JSType.
253 JSValue* m_value; // This is not a real JSValue, only ever a JSImmediate, thus no ProtectedPtr
I would word it as "never a JSCell", rather than "not a real JSValue".
I don't understand the line of code in the LocalVarTypeOfNode constructor that sets m_expectedReturnType to StringType. Isn't it guaranteed to already be StringType?
It seems AddNodeNumbers should be AddNumberNode, and AddNodeStrings should be AddStringsNode. For AddNodeStringLeft and AddNodeStringRight, AddStringLeftNode and AddStringRightNode are not great names, but probably OK unless we can think of something better.
r=me as long as you fix the m_expectedReturnType issue.
Darin and I discussed all the final changes over IRC. I ran the tests again when landing, and found SunSpider actually reported the final patch as a 1.1% speedup. |