[webkit-reviews] review denied: [Bug 226418] Optimize Function.prototype.toString : [Attachment 430475] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Tadeu Zagallo <tzagallo at apple.com>'s
request for review:
Bug 226418: Optimize Function.prototype.toString
https://bugs.webkit.org/show_bug.cgi?id=226418

Attachment 430475: Patch

https://bugs.webkit.org/attachment.cgi?id=430475&action=review




--- Comment #10 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 430475
  --> https://bugs.webkit.org/attachment.cgi?id=430475
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430475&action=review

Mostly LGTM, but I found a bug.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2867
> +		       asString =
static_cast<NativeExecutable*>(function->executable())->asString();
> +		   asString = function->jsExecutable()->asString();

this looks wrong, and like it should crash for host functions in a debug
assert, as you call jsExecutable. Can you make sure we have a test? I think it
should be trivial-ish by just calling toString on a builtin constructor

To fix, I think you just need to put it in an else.

Also, a small comment on organization, you could add a method to JSFunction
called "asStringConcurrently" or similar, that's called from AI. The details
will go in there, which are maybe slightly nicer?

> Source/JavaScriptCore/dfg/DFGClobberize.h:1823
> +	   read(JSCell_structureID);

I wonder if this is actually necessary. It's a bit deceptive, but we don't
actually care if the StructureID actually changes. Since we're reading the
class info, that's guaranteed to never change via transitions. So we're ok if
someone writes structure in a loop, that shouldn't really prevent us from being
hoisted out of said loop

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10537
> +    slowCases.append(m_jit.branchPtr(CCallHelpers::Equal, result.gpr(),
TrustedImmPtr(JSBoundFunction::info())));

nit: maybe it's worth a static assert that JSBoundFunction is final? That
guarantees this branch has to be correct.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8251
> +	   m_out.branch(m_out.equal(classInfo,
m_out.constIntPtr(JSBoundFunction::info())), rarely(slowCase),
usually(notBoundFunctionCase));

nit: I don't think we should prescribe this a-priori as a slow case. Maybe the
program really is toStringing a bound function

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:126
> +    auto throwScope = DECLARE_THROW_SCOPE(vm);
> +    auto catchScope = DECLARE_CATCH_SCOPE(vm);

ditto about just needing throwScope

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:130
> +	       return ::JSC::asString(jsMakeNontrivialString(globalObject,
"function ", name().string(), "() {\n	 [native code]\n}"));

if this throws, asString is the wrong thing...

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:183
> +	   return ::JSC::asString(jsMakeNontrivialString(globalObject,
functionHeader, name, src));

ditto about OOM and asString being wrong

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:189
> +    if (catchScope.exception()) {
> +	   throwScope.throwException(globalObject, catchScope.exception());
> +	   return nullptr;
> +    }

I think this can just be 
RETURN_IF_EXCEPTION(throwScope, nullptr)

> Source/JavaScriptCore/runtime/FunctionExecutable.h:289
> +    JSString* asString() const

maybe "asStringConcurrently"? AFAICT, the only use is the compiler. So it's
helpful to document that it can be used in that context.

> Source/JavaScriptCore/runtime/JSFunction.cpp:255
> +	   return asString(jsMakeNontrivialString(globalObject, "function ",
function->nameString(), "() {\n    [native code]\n}"));

this is wrong if OOM, it won't be a string

> Source/JavaScriptCore/runtime/NativeExecutable.cpp:101
> +    auto throwScope = DECLARE_THROW_SCOPE(vm);
> +    auto catchScope = DECLARE_CATCH_SCOPE(vm);

this is a non idiomatic way of doing things. I don't get why you need the catch
scope at all. I think the throw scope itself is sufficient. After the call to
asString, I'd just do:
RETURN_IF_EXCEPTION(throwScope, nullptr);

> JSTests/microbenchmarks/function-to-string.js:22
> +function test() {

Maybe also good to have some microbenchmarks for:
- CSE
- LICM