Bug 226418 – Optimize Function.prototype.toString

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

<rdar://77861846>

[Tadeu Zagallo]

Created attachment 430096 [details]
Patch

[Filip Pizlo]

Comment on attachment 430096 [details]
Patch

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

Looks good, but your OSR exit should really be a slow path.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10522
> +    speculationCheck(BadType, JSValueSource::unboxedCell(result.gpr()), node, m_jit.branchIfEmpty(result.gpr()));

This should be a slow path.

You don't want to OSR exit just because you got passed a function that was stringified yet.  OSR exits are far too expensive to be part of the mechanism by which functions get stringified for the first time.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8233
> +        speculate(
> +            BadType, jsValueValue(asString), m_node,
> +            m_out.isNull(asString));

Slow path please.

[Tadeu Zagallo]

Created attachment 430265 [details]
Patch

[Keith Miller]

Comment on attachment 430265 [details]
Patch

Looks like a test is berked.

[Saam Barati]

Comment on attachment 430265 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        Make it an intrinsic and add support for it in DFG and FTL. I added a new
> +        microbenchmark that speeds up by >40x;

This explanation can be a lot more descriptive. The important speedup isn't from making it an intrinsic. It's from caching it. This makes no mention of that.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2859
> +        setForNode(node, m_vm.stringStructure.get());

any chance we can constant fold this in the compiler if the executable is a constant and its m_asString is set?

Maybe to make that safe, we'd need a storeStoreFence before storing to m_asString of Executable.

> Source/JavaScriptCore/dfg/DFGDoesGC.cpp:115
> +    case ExecutableToString:

this is wrong

> Source/JavaScriptCore/dfg/DFGNodeType.h:423
> +    macro(ExecutableToString, NodeResultJS | NodeMustGenerate) \

Why must generate?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10522
> +    JITCompiler::Jump slowPath = m_jit.branchIfEmpty(result.gpr());

branchIfEmpty is for JSValues. You should be using a branchTestPtr here. (Logically they're the same on 64-bit, but this aint right on 32-bit)

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:80
> +    visitor.append(thisObject->m_asString);

why not put this in rare data?

> Source/JavaScriptCore/runtime/FunctionExecutable.h:325
> +    JSValue toStringSlow(JSGlobalObject*);

this should return JSString*, not JSValue

[Saam Barati]

Comment on attachment 430265 [details]
Patch

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

> Source/JavaScriptCore/runtime/FunctionExecutableInlines.h:42
> +        m_asString.set(vm, this, asString(toStringSlow(globalObject)));

what if this throws? Then what're we setting this to? I feel like we need to only set this if toStringSlow doesn't throw.

[Saam Barati]

Comment on attachment 430265 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8245
> +        setJSValue(m_out.phi(Int64, fastResult, slowResult));

nit: I think Int64 => pointerType() is a bit more semantically correct here

[Saam Barati]

Comment on attachment 430265 [details]
Patch

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

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:84
>              RELEASE_AND_RETURN(scope, JSValue::encode(jsMakeNontrivialString(globalObject, "function ", function->name(vm), "() {\n    [native code]\n}")));

why aren't we caching for host/builtin?

[Tadeu Zagallo]

Created attachment 430475 [details]
Patch

[Saam Barati]

Comment on attachment 430475 [details]
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

[Tadeu Zagallo]

Created attachment 430513 [details]
Patch

[Tadeu Zagallo]

Created attachment 430514 [details]
Patch

[Saam Barati]

Comment on attachment 430514 [details]
Patch

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

Nice. r=me if you fix the final OOM bug with jsMakeNontrivialString and asString-ing it.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10533
> +    speculateCellType(node->child1(), function.gpr(), SpecFunction, JSFunctionType);

nit: I'd call speculateFunction(node->child1(), function.gpr()) here, so we're not repeating the contents of it.

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

still the same bug as before regarding OOM.

[Tadeu Zagallo]

Comment on attachment 430514 [details]
Patch

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

Thanks for the reviews

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10533
>> +    speculateCellType(node->child1(), function.gpr(), SpecFunction, JSFunctionType);
> 
> nit: I'd call speculateFunction(node->child1(), function.gpr()) here, so we're not repeating the contents of it.

oh, that's better, I copied this from compileGetExecutable, I'll update both places.

>> Source/JavaScriptCore/runtime/JSFunction.cpp:255
>> +        return asString(jsMakeNontrivialString(globalObject, "function ", function->nameString(), "() {\n    [native code]\n}"));
> 
> still the same bug as before regarding OOM.

missed this one, will fix before landing.

[Tadeu Zagallo]

Created attachment 430579 [details]
Patch for landing

[EWS]

Committed r278462 (238482@main): <https://commits.webkit.org/238482@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430579 [details].