[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


Mark Rowe (bdash) <mrowe at apple.com> changed:

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




--- Comment #2 from Mark Rowe (bdash) <mrowe at apple.com>  2014-04-10 17:45:59 PST ---
(From update of attachment 229090)
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-statements>

> 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-statements>

> 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-statements>

> 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.

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