[webkit-reviews] review denied: [Bug 131515] Inlining native functions into the JavaScript in the FTL : [Attachment 229090] Inlines native functions into JavaScript code in the FTL

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe 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 229090: Inlines native functions into JavaScript code in the FTL
https://bugs.webkit.org/attachment.cgi?id=229090&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229090&action=review


> Source/JavaScriptCore/ChangeLog:4
> +	   Also fixed the build to not compress the bitcode and to include all
of the relevant runtime.

ChangeLog entries are intended to explain why changes are being made, not just
what they're doing. For instance, why is it ok for these not to be compressed?
Clearly it was considered a good idea at some point.

> Source/JavaScriptCore/ChangeLog:16
> +	   * dfg/DFGByteCodeParser.cpp:
> +	   (JSC::DFG::ByteCodeParser::handleCall):
> +	   * dfg/DFGNode.h:
> +	   (JSC::DFG::Node::predictHeap):

In case it's not clear, these function-level entries in the ChangeLog exist to
be filled out with details about the change.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1233
> +    } else if (JSFunction* function = callLinkStatus.function())
> +	   if (function->isHostFunction()) {
> +	       emitFunctionChecks(callLinkStatus, callTarget, registerOffset,
kind);
> +	       knownFunction = function;
> +	   }

The else if needs a brace around its body.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:51
> +
> +#include <llvm/InitializeLLVM.h>
>  #include <atomic>
>  #include <wtf/ProcessID.h>
> +#include <dlfcn.h>

There shouldn't be a blank line here, and these should be sorted.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:146
> +			   if (dladdr((void *)func, &info)) {

void*

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:148
> +			       if (callee && numArgs > max_numArgs) max_numArgs
= numArgs;

<http://www.webkit.org/coding/coding-style.html#linebreaking-multiple-statement
s>

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:160
> +	   if (max_numArgs >= 0){

){ -> ) {

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:218
> +	   if (m_ftlState.nativeLoadedLibraries.contains(path)) return true;

<http://www.webkit.org/coding/coding-style.html#linebreaking-multiple-statement
s>

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:244
> +	   bool linkSuccess = 
> +	       !llvm->LinkModules(m_ftlState.module, module,
LLVMLinkerDestroySource, nullptr);

This seems like unnecessary wrapping. I'd be inclined to remove the local and
move the call directly in to the if.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:255
> +	   if (!m_ftlState.symbolTable.contains(symbol)) return nullptr;
> +	   if (!getModuleByPath(m_ftlState.symbolTable.get(symbol))) return
nullptr;

<http://www.webkit.org/coding/coding-style.html#linebreaking-multiple-statement
s>

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3607
> +	   if (m_node->hasKnownFunction()) {

Code within this if code do with being extracted in to a helper function with a
name that explains what it does.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3611
> +	       if (dladdr((void *)func, &info)) {

void*.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3629
> +		       for (int i = 0; i < numPassedArgs; ++i)
> +			  
m_out.storePtr(lowJSValue(m_graph.varArgChild(m_node, 1 + i)),
> +			       addressFor(m_execStorage,
JSStack::FirstArgument, i * sizeof(Register))); // 8 for number of bytes in a
long

This should have braces around the body since it's more than one line long.

The comment doesn't seem to fit the code.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3637
> +			   m_out.bitCast(m_out.address(m_execState,
m_heaps.CallFrame_callerFrame ).value(), tyCalleeArg));
> +		       //setInstructionCallingConvention(call,
LLVMWebKitJSCallConv);

Commented-out code should be removed.

> Source/JavaScriptCore/ftl/FTLState.cpp:38
> +#include <stdio.h>
> +#include <wtf/text/StringBuilder.h>
> +#include <wtf/text/WTFString.h>
> +#include <llvm/InitializeLLVM.h>

These should be sorted.

> Source/JavaScriptCore/ftl/FTLState.cpp:61
> +#if CPU(X86_64)
> +	   "/x86_64/Runtime.symtbl"
> +#elif CPU(ARM64)
> +	   "/arm64/Runtime.symtbl"
> +#else
> +#error "Unrecognized Architecture"
> +#endif

We're repeating this pattern in a few places. Can we share the code rather than
duplicating it?

> Source/JavaScriptCore/ftl/FTLState.cpp:63
> +    FILE * symFile = fopen(fullPath.data(), "r");

FILE*

"Use full words, except in the rare case where an abbreviation would be more
canonical and easier to understand"
(<http://www.webkit.org/coding/coding-style.html#names-full-words>)

> Source/JavaScriptCore/ftl/FTLState.cpp:70
> +	   StringBuilder str;
> +	   char c;
> +	   while ((c = getc(symFile)) != EOF)
> +	       str.append(c);
> +	   fclose(symFile);

One byte at a time is an awfully inefficient way to read a file from disk.

> Source/JavaScriptCore/ftl/FTLState.cpp:80
> +	   Vector<String> lines;
> +	   Vector<String> symbol_loc;	     
> +	   file.split('\n', lines);
> +	   size_t end = lines.size();
> +	   for (size_t i = 0; i < end; i++) {
> +	       lines[i].split(' ', symbol_loc);
> +	       symbolTable.set(symbol_loc[0].ascii(), symbol_loc[1].ascii());
> +	   }

This isn't particularly efficient either. You're allocating a vector large
enough to hold all of the lines, even though you're processing them one line at
a time.

symbol_loc isn't a good variable name either
(<http://www.webkit.org/coding/coding-style.html#names-basic>,
<http://www.webkit.org/coding/coding-style.html#names-full-words>).

> Source/JavaScriptCore/ftl/FTLState.cpp:83
> +    } else {
> +	   dataLog("Could not open symbol table from ", fullPath, " but
continuing anyway\n");
> +    }

The braces are unnecessary around a one-line block like this.

> Source/JavaScriptCore/ftl/FTLState.h:86
> +    HashSet<CString> nativeLoadedLibraries;
> +    HashMap<CString, CString> symbolTable;

Data members typically have an m_ prefix. I wonder why this class is special?

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:45
> +const char* nativeLibraryPath;

Should be static?

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:55
> +	   NSString *path = [[myBundle bundlePath]
stringByAppendingPathComponent:@"Libraries/libllvmForJSC.dylib"];	 
> +	   nativePath = [[myBundle bundlePath]
stringByAppendingPathComponent:@"Resources/Runtime"];
> +	   [nativePath retain];
>	   initializeLLVMPOSIX([path UTF8String]);

The ordering of these statements is a little odd. Your new code should probably
live after the call to initializeLLVMPOSIX so there's a more obvious connection
between initializating path and that call.

The way this is written currently it's leaking nativePath in order to keep
alive the UTF8 representation of it. It'd make more sense for the global to be
of type NSString.

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:63
> +const char * getNativeLibraryPath()
> +{
> +    return nativeLibraryPath;
>  }

The get prefix isn't necessary. We typically only use it for functions that
return values via an out argument.

The return type should be const char*.

"native library path" seems like an odd name for the path that contains bitcode
files.