[Webkit-unassigned] [Bug 131515] Inlining native functions into the JavaScript in the FTL

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=131515


Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #230236|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #25 from Filip Pizlo <fpizlo at apple.com>  2014-04-27 20:44:51 PST ---
(From update of attachment 230236)
View in context: https://bugs.webkit.org/attachment.cgi?id=230236&action=review

> LayoutTests/js/regress/script-tests/ftl-library-inlining.js:11
> +function foo(x, d){
> +    return x + Math.random() + d.getTimezoneOffset();
> +}
> +
> +noInline(foo);
> +
> +for (var i = 0 ; i < 100000000; i++){
> +    foo(i, new Date());
> +}
> +
> +

What is the speed-up on this?

You should prepare-ChangeLog for LayoutTests.

> Source/JavaScriptCore/ChangeLog:2055
> +2014-04-11  Matthew Mirman  <mmirman at apple.com>
> +
> +        Added system for inlining native functions via the FTL.
> +        https://bugs.webkit.org/show_bug.cgi?id=131515
> +
> +        Reviewed by NOBODY (OOPS!).

You should move this to the top of the ChangeLog.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3577
> +    bool isInlinableSize(LValue function)

It's customary to define all helper functions below all of the compileBlah() functions.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3592
> +    bool getModuleByPathForSymbol(const CString path, const CString symbol)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3597
> +                // we had no choice but to compile this function, but don't try to inline it ever again.

I would capitalize "We" at the start of this sentence.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3652
> +#if !ASSERT_DISABLED
> +                llvm->RemoveFunctionAttr(function, LLVMStackProtectAttribute);
> +#endif // !ASSERT_DISABLED

I would instead say if (!ASSERT_DISABLED) rather than #if !ASSERT_DISABLED.  That's the style elsewhere in this file.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3667
> +    LValue getFunctionBySymbol(const CString symbol)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3676
> +    bool possiblyCompileInlineableNativeCall(int dummyThisArgument, int numPassedArgs, int numArgs)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3707
> +                llvm->GetParamTypes(typeCallee, &typeCalleeArg);
> +                LValue call = m_out.call(callee, 
> +                    m_out.bitCast(m_out.address(m_execState, m_heaps.CallFrame_callerFrame).value(), typeCalleeArg));

This doesn't appear to handle exceptions.

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:65
> +
> +        NSString *nativePath = [[myBundle bundlePath] stringByAppendingPathComponent:@
> +#if CPU(X86_64)
> +            "Resources/Runtime/x86_64"
> +#elif CPU(ARM64) 
> +            "Resources/Runtime/arm64"
> +#else
> +#error "Unrecognized Architecture"
> +#endif
> +            ];
> +        llvmBitcodeLibraryForInliningPath = new CString([nativePath UTF8String]);

This is the wrong place for this.  initializeLLVM() is for initializing the LLVM libraries and shouldn't have FTL-related logic.  It certainly shouldn't be the catch-all place for everyone who needs the bundle path.

Can you instead factor out the bundle path logic into a separate thing (maybe, runtime/ResourcePaths.h|mm|cpp)?  InitializeLLVM would then use it, and your native code thingy can also use it.

> Source/JavaScriptCore/runtime/JSObject.h:608
> +    JS_EXPORT_PRIVATE bool isSealed(VM& vm) { return structure(vm)->isSealed(vm); }
> +    JS_EXPORT_PRIVATE bool isFrozen(VM& vm) { return structure(vm)->isFrozen(vm); }
> +    JS_EXPORT_PRIVATE bool isExtensible() { return structure()->isExtensible(); }

You should investigate why these need to be exported.  Can you explain it?

> Source/JavaScriptCore/tests/stress/ftl-library-exception.js:19
> +function foo(d){
> +    return d.getTimezoneOffset();
> +}
> +
> +noInline(foo);
> +
> +var x;
> +var count = 1000000;
> +for (var i = 0 ; i < count; i++){
> +    try { 
> +        foo(i < count - 100 ? new Date() : "a");
> +        x = false;
> +    } catch (e) {
> +        x = true;
> +    }
> +}
> +
> +if (!x)
> +    throw "bad result: "+ x;

I'm not convinced that this tests exception handling for native calls.  The exception is probably thrown when we OSR exit on CheckFunction.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.