[webkit-reviews] review granted: [Bug 15884] JavaScriptCore should use simple type inferencing to speed-up AddNode : [Attachment 17181] Patch updated to TOT and addressing Darin's concerns

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 15884: JavaScriptCore should use simple type inferencing to speed-up
AddNode
http://bugs.webkit.org/show_bug.cgi?id=15884

Attachment 17181: Patch updated to TOT and addressing Darin's concerns
http://bugs.webkit.org/attachment.cgi?id=17181&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
 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.