[webkit-reviews] review granted: [Bug 19721] speed up JavaScriptCore by not wrapping strings in objects just to call functions on them : [Attachment 21941] patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 19721: speed up JavaScriptCore by not wrapping strings in objects just to
call functions on them
https://bugs.webkit.org/show_bug.cgi?id=19721

Attachment 21941: patch
https://bugs.webkit.org/attachment.cgi?id=21941&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
+2008-06-25  Darin Adler  <darin at apple.com>
+
+	 Reviewed by NOBODY (OOPS!).
+
+	 * ChangeLog:
+	 * JavaScriptCore.exp:
+	 * VM/JSPropertyNameIterator.cpp:
+	 * VM/Machine.cpp:


Looks like you got two ChangeLogs for the price of one.

+JSObject* JSImmediate::prototype(const JSValue* v, ExecState* exec)
+{
+    ASSERT(isImmediate(v));
+    if (v == jsNull())
+	 return new (exec) JSNotAnObject(throwError(exec, TypeError, "Null
value"));
+    if (v == jsUndefined())
+	 return new (exec) JSNotAnObject(throwError(exec, TypeError, "Undefined
value"));
+    if (isBoolean(v))
+	 return exec->lexicalGlobalObject()->booleanPrototype();
+    ASSERT(isNumber(v));
+    return exec->lexicalGlobalObject()->numberPrototype();
+}

It surprised me that you put the ifs for error cases before the ifs for normal
cases. I would expect number to be most likely, followed by boolean, undefined,
null. Maybe that order would speed things up a little.

I will trade not asking you to split this patch up in exchange for asking you
for a couple of string replacement regression tests (unless there's a bunch of
coverage in the Mozilla tests?)

r=me
Geoff