[webkit-reviews] review denied: [Bug 131515] Inlining native functions into the JavaScript in the FTL : [Attachment 230236] FTL native library inlining system

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 131515: Inlining native functions into the JavaScript in the FTL
https://bugs.webkit.org/show_bug.cgi?id=131515

Attachment 230236: FTL native library inlining system
https://bugs.webkit.org/attachment.cgi?id=230236&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.