|
Description
Matthew Mirman
2014-04-10 16:58:15 PDT
Created attachment 229090 [details]
Inlines native functions into JavaScript code in the FTL
It leaves when to do it up to LLVM. Also removed the gzip stuff and fixed a bug in symbol table generation and added back all the runtime files to the "Compile Runtime To LLVM IR" target.
Comment on attachment 229090 [details] Inlines native functions into JavaScript code in the FTL 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. Comment on attachment 229090 [details] Inlines native functions into JavaScript code in the FTL View in context: https://bugs.webkit.org/attachment.cgi?id=229090&action=review Since this is a change that should have an observable effect on performance, it should come with some sort of performance test to verify its behavior. It's OK if the test is very basic -- like just calling Math.hypot or window.isNaN in a loop. Ideally, you should test a few examples like this. Any regression test you add to LayoutTests/js/regress will run as a performance test in run-jsc-benchmarks --js-regress. > Source/JavaScriptCore/runtime/JSCJSValue.h:197 > + JS_EXPORT_PRIVATE bool isInt32() const; > + JS_EXPORT_PRIVATE bool isUInt32() const; > + JS_EXPORT_PRIVATE bool isDouble() const; > + JS_EXPORT_PRIVATE bool isTrue() const; > + JS_EXPORT_PRIVATE bool isFalse() const; This doesn't look quite right. "JS_EXPORT_PRIVATE" implies that somebody emitted a true call to one of these functions, which the linker needed to resolve. But you really don't want to emit any calls to these functions. Instead, you want them to be inlined via JSCJSValueInlines.h. Created attachment 229180 [details]
System for inlining native functions via the FTL.
Addressed Mark Rowe's concerns.
Attachment 229180 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:130: max_numArgs is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:147: More than one command on the same line in if [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3584: Extra space before ) [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/llvm/LLVMHeaders.h:49: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLState.cpp:57: line_allocated_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLState.cpp:58: line_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLState.cpp:66: Semicolon defining empty statement for this loop. Use { } instead. [whitespace/semicolon] [5]
Total errors found: 7 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229180 [details] System for inlining native functions via the FTL. View in context: https://bugs.webkit.org/attachment.cgi?id=229180&action=review Does this have heuristics for deciding not to inline functions that are too big? What is the performance impact? > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:126 > + > + I don't think we should have a double-space here. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3560 > + LType tyCallee; "tyCallee" isn't a very good variable name. We don't usually use abbreviations in WebKit. "calleeType" would be better. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3584 > + llvm->GetParamTypes(tyCallee, &tyCalleeArg); > + LValue call = m_out.call(callee, > + m_out.bitCast(m_out.address(m_execState, m_heaps.CallFrame_callerFrame ).value(), tyCalleeArg)); This doesn't actually inline, right? If so, you should either change the name of this bug, or implement the inlining. That probably involves adding some inlining phases to the LLVM pipeline in FTLCompile.cpp. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3601 > + return; This should be indented 4 fewer spaces. > Source/JavaScriptCore/ftl/FTLOutput.h:213 > + LValue allocaName(LType type, const char* nm) { return llvm->BuildAlloca(m_builder, type, nm); } Why do you need to use the name? We usually don't name LLVM instructions. Also, it seems like you don't use this. Created attachment 229183 [details]
System for inlining native functions via the FTL.
(In reply to comment #6) > (From update of attachment 229180 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229180&action=review > > Does this have heuristics for deciding not to inline functions that are too big? I'm relying on the LLVM's heuristics to do that. http://llvm.org/docs/doxygen/html/InlinerPass_8h_source.html > What is the performance impact? I'm not exactly sure. Loading the symbol table is a pretty trivial one time cost. Loading each llvm module appears to be pretty quick and is also a one time cost per module. > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3584 > > + llvm->GetParamTypes(tyCallee, &tyCalleeArg); > > + LValue call = m_out.call(callee, > > + m_out.bitCast(m_out.address(m_execState, m_heaps.CallFrame_callerFrame ).value(), tyCalleeArg)); > > This doesn't actually inline, right? If so, you should either change the name of this bug, or implement the inlining. That probably involves adding some inlining phases to the LLVM pipeline in FTLCompile.cpp. I'm not certain, but I'm pretty sure LLVM does the inlining in the optimization pass. Remember when we were debugging the segfault on the random inline test and noticed that the resulting assembly for "foo" was gigantic? I was under the impression that this was because "random" had been inlined. Of course, the heuristics could get more interesting since we know something about the heat of individual paths, and llvm includes a tag which you can paste to a function in a module which tells it either to always inline or never inline it (I wonder if the "always inline" is as much of a lie as it is for C). Created attachment 229576 [details]
System for inlining native functions via the FTL.
Added the actual inlining pass.
Fixed the allocation size bug.
Added passes to remove unused functions from imported modules. This potentially speeds up optimization and can be very useful for debugging.
As suggested by llvmers, added "AnalysisPass" before other passes in the simpleOptimization case.
Attachment 229576 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/llvm/LLVMHeaders.h:55: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:58: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:60: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 3 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229576 [details]
System for inlining native functions via the FTL.
Do you have a mechanism for preventing the inlining of huge functions? Have you run any benchmarks to measure performance impact? Have you run the full test suite to ensure that there aren't correctness regressions?
Created attachment 229589 [details]
System for inlining native functions via the FTL.
Fixed style issues.
Changed do-whiles over iterators into for loops.
Added pass to ensure large functions don't get inlined.
Attachment 229589 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3580: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3615: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 2 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229589 [details] System for inlining native functions via the FTL. View in context: https://bugs.webkit.org/attachment.cgi?id=229589&action=review Have you done performance measurements? Have you run-javascriptcore-tests? Have you run-webkit-tests? > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3512 > + if (++instructionCount >= 275) You should turn 275 into an Options::maximumLLVMInstructionCountForNativeInlining(). See runtime/Options.h. Add a new option there and make 275 the default. Where does 275 come from? (In reply to comment #14) > (From update of attachment 229589 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229589&action=review > > Have you done performance measurements? Have you run-javascriptcore-tests? Have you run-webkit-tests? > > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3512 > > + if (++instructionCount >= 275) > > You should turn 275 into an Options::maximumLLVMInstructionCountForNativeInlining(). See runtime/Options.h. Add a new option there and make 275 the default. > > Where does 275 come from? 275 is the default for LLVM's inlining threshold. I have absolutely no idea how they actually compare, or how LLVM gets the number to compare to its 275. Created attachment 229594 [details]
System for inlining native functions via the FTL.
Generalized maximumLLVMInstructionCountForNativeInlining
Fixed style.
Created attachment 230023 [details]
system for inlining native functions via the FTL.
WIP
Created attachment 230201 [details]
FTL inlining system
removes the loading of a file at runtime by including a list of hashtable adds.
Created attachment 230202 [details] Benchmark for https://bug-131515-attachments.webkit.org/attachment.cgi?id=230201 Proves that its at least not slower. Created attachment 230208 [details] javascriptcore test results for https://bug-131515-attachments.webkit.org/attachment.cgi?id=230201 Passed all tests. Attachment 230201 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ftl/FTLState.cpp:53: Bad include order. Mixing system and custom headers. [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3595: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 2 in 34 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 230222 [details]
FTL native library inlining system
Still needs a microbenchmark suite, but running "ftl-library-inlining-random.js" with "time" with inlining on and off shows as much as a 20% speedup.
Also, added a test for native exceptions.
Created attachment 230230 [details]
FTL native library inlining system
Added a benchmarking test, removed thtroublesom e js_private_export.
Created attachment 230236 [details]
FTL native library inlining system
Fixed the build bug.
Comment on attachment 230236 [details] FTL native library inlining system 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@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. Created attachment 230332 [details]
Native runtime library inlining in FTL
Fixed some style, moved some files.
Have not yet been able to respond to exception throwing critique.
(In reply to comment #25) > (From update of attachment 230236 [details]) > 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? 13448.2029+-314.2389 ^ 12229.0609+-107.4231 ^ definitely 1.0997x faster > > 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? At the moment they aren't all exactly needed but could be in the future. isExtensible is definitely needed since one of the functions uses it and is a native js function which is tested. The other two are connected to javascript runtime functions which could be lowered in the future but aren't currently tested. Created attachment 230608 [details]
Native runtime library inlining in FTL
test results forthcoming.
Attachment 230608 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3748: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 42 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 230610 [details]
Native runtime library inlining in FTL
Fixed style
Created attachment 230611 [details] Test results for https://bug-131515-attachments.webkit.org/attachment.cgi?id=230610 (In reply to comment #31) > Created an attachment (id=230611) [details] > Test results for https://bug-131515-attachments.webkit.org/attachment.cgi?id=230610 It's not necessary to post test results. Only post benchmark results. Also, you should Tools/Scripts/run-webkit-tests --debug Created attachment 230613 [details] JS Regress benchmark for https://bug-131515-attachments.webkit.org/attachment.cgi?id=230610 Created attachment 230624 [details] All benchmarks for https://bug-131515-attachments.webkit.org/attachment.cgi?id=230610 Created attachment 230696 [details]
Native runtime library inlining in FTL
Removed some errant JS_EXPORT_PRIVATEs which fixed building with --debug.
Ran run-javascript-core tests with no errors.
Ran "run-webkit-tests --debug" with the following errors which I believe to be preexisting and not results of this patch (please review):
[1429/33776] compositing/hidpi-compositing-vs-non-compositing-check-on-testing-framework.html failed unexpectedly (reference mismatch)
[2389/33776] compositing/hidpi-non-simple-compositing-layer-with-fractional-size-and-background.html failed unexpectedly (reference mismatch)
[5469/33776] fast/backgrounds/gradient-background-shadow.html failed unexpectedly (reference mismatch)
[9950/33776] fast/dom/shadow/content-element-outside-shadow.html passed unexpectedly
fast/gradients/css3-color-stop-invalid.html -> ref test hashes didn't match but diff passed
[12549/33776] fast/images/pdf-as-image-crop-box.html failed unexpectedly (reference mismatch)
[12797/33776] fast/multicol/newmulticol/clipping.html passed unexpectedly
[13804/33776] fast/scrolling/scroll-select-list.html passed unexpectedly
[14333/33776] fast/sub-pixel/file-upload-control-at-fractional-offset.html passed unexpectedly
[16550/33776] fast/workers/worker-call.html passed unexpectedly
[16845/33776] inspector-protocol/profiler/console-profile.html failed unexpectedly (text diff)
[17855/33776] mathml/wbr-in-mroot-crash.html passed unexpectedly
[20007/33776] mathml/presentation/mspace-units.html passed unexpectedly
[21389/33776] media/media-controls-invalid-url.html passed unexpectedly
[22836/33776] jquery/manipulation.html failed unexpectedly (text diff)
svg/css/svg-resource-fragment-identifier-img-src.html -> ref test hashes didn't match but diff passed
svg/stroke/non-scaling-stroke-pattern.svg -> ref test hashes didn't match but diff passed
[31391/33776] webgl/1.0.2/conformance/ogles/GL/build/build_009_to_016.html passed unexpectedly
[31762/33776] webgl/1.0.2/conformance/glsl/misc/shader-uniform-packing-restrictions.html failed unexpectedly (text diff)
[33721/33776] http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-synconmain.html failed unexpectedly (test timed out, text diff)
Created attachment 232445 [details]
Native runtime library inlining in FTL for ftlopt branch
for ftlopt branch.
Comment on attachment 232445 [details] Native runtime library inlining in FTL for ftlopt branch View in context: https://bugs.webkit.org/attachment.cgi?id=232445&action=review > LayoutTests/ChangeLog:13 > + * js/regress/script-tests/ftl-library-inlining.js: Added. > + * js/regress/script-tests/ftl-library-inlining-dataview.js: Added. > + * js/regress/script-tests/ftl-library-inlining-exceptions.js: Added. > + * js/regress/script-tests/ftl-library-inlining-folding.js: Added. > + * js/regress/script-tests/ftl-library-inlining-loops.js: Added. These tests should check errors. > Source/JavaScriptCore/runtime/BundlePath.h:34 > +const CString * bundlePath(); Change this to simply return const CString& or even just CString. Or a const char*. Returning a const CString* is weird. Created attachment 232447 [details]
Native runtime library inlining in FTL for ftlopt branch
Adds error checking for new benchmarks.
Comment on attachment 232447 [details] Native runtime library inlining in FTL for ftlopt branch View in context: https://bugs.webkit.org/attachment.cgi?id=232447&action=review > LayoutTests/js/regress/script-tests/ftl-library-inlining-dataview.js:13 > + result = foo(new DataView(new ArrayBuffer(5))); It would be far better to make this a result += foo..., so that your checking incorporates results from runs 0 through 3999999 and not just the last run. > LayoutTests/js/regress/script-tests/ftl-library-inlining-exceptions.js:15 > + try { > + foo(i < count - 1000 ? new Date() : "a"); > + x = false; > + } catch (e) { > + x = true; > + } Ditto. You could have a result that is computed by adding up the x's. > LayoutTests/js/regress/script-tests/ftl-library-inlining-folding.js:14 > + result = foo(); Ditto. Created attachment 232448 [details]
Native runtime library inlining in FTL for ftlopt branch
Makes error checking sum over results in benchmarking tests.
Comment on attachment 232448 [details]
Native runtime library inlining in FTL for ftlopt branch
You forgot the .html and -expected.txt boilerplate for the LayoutTests/js/regress tests.
Created attachment 232451 [details]
Native runtime library inlining in FTL for ftlopt branch
Added boilerplate for tests
Comment on attachment 232451 [details]
Native runtime library inlining in FTL for ftlopt branch
I'm gonna try to land this.
Landed in http://trac.webkit.org/changeset/169578 I just discovered a really bad behavior of this patch: InlineRuntimeSymbolTable.h is checked in and appears to need to be checked in, but the code changes it every time you build. You shouldn't do that, because now it means that anytime someone does a build, their tree will appear to have a diff on that file. I'm going to roll this out. Matt, can you fix this? Comment on attachment 232451 [details] Native runtime library inlining in FTL for ftlopt branch View in context: https://bugs.webkit.org/attachment.cgi?id=232451&action=review > Source/JavaScriptCore/ChangeLog:23 > + Rather than loading Runtime.symtbl at runtime FTLState.cpp now includes a file > + InlineRuntimeSymbolTable.h which statically builds the symbol table hash table. > + Currently build-symbol-table-index.py updates this file from the > + contents of tested-symbols.symlst when done building as a matter of convenience. > + However, in order to include the new contents of the file in the build > + you'd need to build twice. This will be fixed in future versions. We shouldn't do this. Sorry, I hadn't realized the full effect of this. Every time someone builds, they will get a different InlineRuntimeSymbolTable.h and it will show up in their diff. This pretty much breaks the typical WebKit development workflow. Things that are checked into the repository shouldn't be automagically mutated by the build system. Can you remove this feature and post a new patch? > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3677 > +#if CPU(X86_64) > + "/Resources/Runtime/x86_64/", > +#elif CPU(ARM64) > + "/Resources/Runtime/arm64/", > +#else > +#error "Unrecognized Architecture" > +#endif You should find a way of saying this that doesn't involve preprocessor #if's. Notice that we have isX86() and isARM64() inline functions in AbstractMacroAssembler.h, and the FTL tends to use those whenever possible. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3733 > +#if ASSERT_DISABLED > + llvm->RemoveFunctionAttr(function, LLVMStackProtectAttribute); > +#endif // ASSERT_DISABLED Change this to just "if (ASSERT_DISABLED) llvm->RemoveFunctionAttr(function, LLVMStackProtectAttribute);" (on two lines, not one, of course). That's the style elsewhere in this file. > Source/JavaScriptCore/ftl/FTLState.cpp:43 > +#define LINE_SIZE 500 No need to use #define. Use "static const unsigned lineSize = 500". Also, I'm not sure where you're using this? Rolled out in r169584. Comment on attachment 232451 [details] Native runtime library inlining in FTL for ftlopt branch View in context: https://bugs.webkit.org/attachment.cgi?id=232451&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3603 > + bool possiblyCompileInlineableNativeCall(int dummyThisArgument, int numPassedArgs, int numArgs) These helper functions shouldn't be in the same block of code as the "void compileBlah()" functions. Please move them to the bottom of the file. (In reply to comment #47) > Rolled out in r169584. ... note that this rollout kept the tests checked in. Created attachment 232502 [details]
Native runtime library inlining in FTL for ftlopt branch
Fixed suggested things in tests, removed some out of date tests, fixed locations of functions in FTLLowerDFGToLLVM, removed InlineRuntimeSymbolTable.h from version control.
Comment on attachment 232502 [details]
Native runtime library inlining in FTL for ftlopt branch
Can you rebase this to exclude the tests? The tests are still in the tree, they were not rolled out.
Created attachment 232507 [details]
Native runtime library inlining in FTL for ftlopt branch
Hopefully fixed already existent tests problem.
Comment on attachment 232507 [details] Native runtime library inlining in FTL for ftlopt branch View in context: https://bugs.webkit.org/attachment.cgi?id=232507&action=review High-level comments: - Please remove all changes to LayoutTests. I changed the tests before rolling out this change. The changes I made were intentional. - It would be better if you added all of the LLVM functions that you are using the FTLAbbreviations.h, so that you can refer to them using the short form, like setLinkage(global, LLVMPrivateLinkage) rather than llvm->SetLinkage(global, LLVMPrivateLinkage). I would expect that FTLLowerDFGToLLVM never uses llvm-> directly. > LayoutTests/ChangeLog:21 > +2014-06-04 Matthew Mirman <mmirman@apple.com> > + > + [ftlopt] Added system for inlining native functions via the FTL. > + https://bugs.webkit.org/show_bug.cgi?id=131515 > + > + Reviewed by NOBODY (OOPS!). > + > + Fixes microbenchmarks. > + > + * js/regress/script-tests/ftl-library-inlining.js: Fixed bounds > + * js/regress/script-tests/ftl-library-inlining-dataview.js: Fixed Bounds > + * js/regress/script-tests/ftl-library-inlining-exceptions.js: Added. > + * js/regress/ftl-library-inlining-exceptions-expected.txt: Added. > + * js/regress/ftl-library-inlining-exceptions.html: Added. > + * js/regress/script-tests/ftl-library-inlining-folding.js: Added. > + * js/regress/ftl-library-inlining-folding-expected.txt: Added. > + * js/regress/ftl-library-inlining-folding-expected.html: Added. > + * js/regress/script-tests/ftl-library-inlining-loops.js: Added. > + * js/regress/ftl-library-inlining-loops-expected.txt: Added. > + * js/regress/ftl-library-inlining-loops.html: Added. > + Please revert. > LayoutTests/ChangeLog:-68 > - * js/regress/script-tests/ftl-library-inlining-exceptions.js: Added. > - * js/regress/ftl-library-inlining-exceptions-expected.txt: Added. > - * js/regress/ftl-library-inlining-exceptions.html: Added. > - * js/regress/script-tests/ftl-library-inlining-folding.js: Added. > - * js/regress/ftl-library-inlining-folding-expected.txt: Added. > - * js/regress/ftl-library-inlining-folding-expected.html: Added. > - * js/regress/script-tests/ftl-library-inlining-loops.js: Added. > - * js/regress/ftl-library-inlining-loops-expected.txt: Added. > - * js/regress/ftl-library-inlining-loops.html: Added. Revert. > LayoutTests/js/regress/ftl-library-inlining-exceptions-expected.txt:10 > +JSRegress/ftl-library-inlining-exceptions > + > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > + > + > +PASS no exception thrown > +PASS successfullyParsed is true > + > +TEST COMPLETE > + Please revert. > LayoutTests/js/regress/ftl-library-inlining-exceptions.html:12 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > +<html> > +<head> > +<script src="../../resources/js-test-pre.js"></script> > +</head> > +<body> > +<script src="../../resources/regress-pre.js"></script> > +<script src="script-tests/ftl-library-inlining-exceptions.js"></script> > +<script src="../../resources/regress-post.js"></script> > +<script src="../../resources/js-test-post.js"></script> > +</body> > +</html> Revert. > LayoutTests/js/regress/ftl-library-inlining-folding-expected.txt:10 > +JSRegress/ftl-library-inlining-folding > + > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > + > + > +PASS no exception thrown > +PASS successfullyParsed is true > + > +TEST COMPLETE > + Revert. > LayoutTests/js/regress/ftl-library-inlining-folding.html:12 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > +<html> > +<head> > +<script src="../../resources/js-test-pre.js"></script> > +</head> > +<body> > +<script src="../../resources/regress-pre.js"></script> > +<script src="script-tests/ftl-library-inlining-folding.js"></script> > +<script src="../../resources/regress-post.js"></script> > +<script src="../../resources/js-test-post.js"></script> > +</body> > +</html> Revert. > LayoutTests/js/regress/ftl-library-inlining-loops-expected.txt:10 > +JSRegress/ftl-library-inlining-loops > + > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > + > + > +PASS no exception thrown > +PASS successfullyParsed is true > + > +TEST COMPLETE > + Revert. > LayoutTests/js/regress/ftl-library-inlining-loops.html:12 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > +<html> > +<head> > +<script src="../../resources/js-test-pre.js"></script> > +</head> > +<body> > +<script src="../../resources/regress-pre.js"></script> > +<script src="script-tests/ftl-library-inlining-loops.js"></script> > +<script src="../../resources/regress-post.js"></script> > +<script src="../../resources/js-test-post.js"></script> > +</body> > +</html> Revert. > LayoutTests/js/regress/script-tests/ftl-library-inlining-dataview.js:13 > (function() { > var result = 0; > var d = new DataView(new ArrayBuffer(5)); > - for (var i = 0; i < 1000000; i++) { > + for (var i = 0; i < 400000; i++) { > d.setInt8(0, 4); > d.setInt8(1, 2); > d.setInt8(2, 6); > d.setInt16(0, 20); > result += d.getInt8(2) + d.getInt8(0); > } > - if (result != 6000000) > + if (result != 2400000) > throw "Bad result: " + result; > -})(); > +})(); Revert. > LayoutTests/js/regress/script-tests/ftl-library-inlining-exceptions.js:19 > +function foo(d){ > + return Date.prototype.getTimezoneOffset.call(d); > +} > + > +noInline(foo); > + > +var x; > +var count = 200000; > +for (var i = 0 ; i < count; i++){ > + try { > + foo(i < count - 1000 ? new Date() : "a"); > + x = false; > + } catch (e) { > + x = true; > + } > +} > + > +if (!x) > + throw "bad result: "+ x; Revert. > LayoutTests/js/regress/script-tests/ftl-library-inlining-folding.js:18 > +function foo(){ > + var d = new DataView(new ArrayBuffer(5)); > + d.setInt8(0, 4); > + d.setInt8(1, 2); > + d.setInt8(2, 6); > + d.setInt16(0, 20); > + return d.getInt8(2) + d.getInt8(0); > +} > + > +noInline(foo); > + > +var result = 0; > +for (var i = 0 ; i < 300000; i++){ > + result += foo(); > +} > + > +if (result != 1800000) > + throw "Bad result: " + result; Revert. > LayoutTests/js/regress/script-tests/ftl-library-inlining-loops.js:28 > +function foo(){ > + var count = 100; > + var d = new DataView(new ArrayBuffer(count)); > + > + for (var i = 0; i < count / 4; i++){ > + d.setInt32(i, i); > + } > + > + for (var i = 0; i < count; i++){ > + d.setInt8(i, i); > + } > + var result = 0; > + for (var i = 0; i < count; i++){ > + result += d.getInt8(i); > + } > + return result; > +} > + > +noInline(foo); > + > +var r = 0; > +for (var i = 0 ; i < 100000; i++){ > + r += foo(); > +} > + > +if (r != 495000000) > + throw "Bad result: " + r; > + Revert. > LayoutTests/js/regress/script-tests/ftl-library-inlining.js:5 > (function() { > - for (var i = 0 ; i < 10000000; i++) > - Math.random(); > + d = new Date(); > + for (var i = 0 ; i < 200000; i++) > + Math.random() + d.getTimezoneOffset(); > })(); Revert. > Source/JavaScriptCore/ChangeLog:-541 > -2014-06-03 Matthew Mirman <mmirman@apple.com> > - > - [ftlopt] Added system for inlining native functions via the FTL. > - https://bugs.webkit.org/show_bug.cgi?id=131515 > - > - Reviewed by Filip Pizlo. > - > - Also fixed the build to not compress the bitcode and to > - include all of the relevant runtime. With GCC_GENERATE_DEBUGGING_SYMBOLS = NO, > - the produced bitcode files are a 100th the size they were before. > - Now we can include all of the relevant runtime files with only a 3mb overhead. > - This is the same overhead as for two compressed files before, > - but done more efficiently (on both ends) and with less code. > - > - Deciding whether to inline native functions is left up to LLVM. > - The entire module containing the function is linked into the current > - compiled JS so that inlining the native functions shouldn't make them smaller. > - > - Rather than loading Runtime.symtbl at runtime FTLState.cpp now includes a file > - InlineRuntimeSymbolTable.h which statically builds the symbol table hash table. > - Currently build-symbol-table-index.py updates this file from the > - contents of tested-symbols.symlst when done building as a matter of convenience. > - However, in order to include the new contents of the file in the build > - you'd need to build twice. This will be fixed in future versions. > - > - * JavaScriptCore.xcodeproj/project.pbxproj: Added back runtime files to compile. > - * build-symbol-table-index.py: Changed bitcode suffix. > - Added inclusion of only tested symbols. > - Added output to InlineRuntimeSymbolTable.h. > - * build-symbol-table-index.sh: Changed bitcode suffix. > - * copy-llvm-ir-to-derived-sources.sh: Removed gzip compression. > - * tested-symbols.symlst: Added. > - * dfg/DFGByteCodeParser.cpp: > - (JSC::DFG::ByteCodeParser::handleCall): > - Now sets the knownFunction of the call node if such a function exists > - and emits a check that during runtime the callee is in fact known. > - * dfg/DFGNode.h: > - Added functions to set the known function of a call node. > - (JSC::DFG::Node::canBeKnownFunction): Added. > - (JSC::DFG::Node::hasKnownFunction): Added. > - (JSC::DFG::Node::knownFunction): Added. > - (JSC::DFG::Node::giveKnownFunction): Added. > - * ftl/FTLAbbreviatedTypes.h: Added a typedef for LLVMMemoryBufferRef > - * ftl/FTLLowerDFGToLLVM.cpp: > - (JSC::FTL::LowerDFGToLLVM::isInlinableSize): Added. Hardcoded threshold to 275. > - (JSC::FTL::LowerDFGToLLVM::getModuleByPathForSymbol): Added. > - (JSC::FTL::LowerDFGToLLVM::getFunctionBySymbol): Added. > - (JSC::FTL::LowerDFGToLLVM::possiblyCompileInlineableNativeCall): Added. > - (JSC::FTL::LowerDFGToLLVM::compileCallOrConstruct): > - Added call to possiblyCompileInlineableNativeCall > - * ftl/FTLOutput.h: > - (JSC::FTL::Output::allocaName): Added. Useful for debugging. > - * ftl/FTLState.cpp: > - (JSC::FTL::State::State): Added an include for InlineRuntimeSymbolTable.h > - * ftl/FTLState.h: Added symbol table hash table. > - * ftl/FTLCompile.cpp: > - (JSC::FTL::compile): Added inlining and dead function elimination passes. > - * heap/HandleStack.h: Added JS_EXPORT_PRIVATE to a few functions to get inlining to compile. > - * InlineRuntimeSymbolTable.h: Added. > - * llvm/InitializeLLVMMac.mm: Deleted. > - * llvm/InitializeLLVMMac.cpp: Added. > - * llvm/LLVMAPIFunctions.h: Added macros to include Bitcode parsing and linking functions. > - * llvm/LLVMHeaders.h: Added includes for Bitcode parsing and linking. > - * runtime/BundlePath.h: Added. > - * runtime/BundlePath.mm: Added. > - * runtime/DateInstance.h: Added JS_EXPORT_PRIVATE to a few functions to get inlining to compile. > - * runtime/DateInstance.h: ditto. > - * runtime/DateConversion.h: ditto. > - * runtime/ExceptionHelpers.h: ditto. > - * runtime/JSCJSValue.h: ditto. > - * runtime/JSArray.h: ditto. > - * runtime/JSDateMath.h: ditto. > - * runtime/JSObject.h: ditto. > - * runtime/JSObject.h: ditto. > - * runtime/RegExp.h: ditto. > - * runtime/Structure.h: ditto. > - * runtime/Options.h: Added maximumLLVMInstructionCountForNativeInlining. > - * tests/stress/ftl-library-inlining-random.js: Added. > - * tests/stress/ftl-library-substring.js: Added. > - Revert. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4104 > +#if ASSERT_DISABLED > + llvm->RemoveFunctionAttr(function, LLVMStackProtectAttribute); > +#endif // ASSERT_DISABLED Please use a normal if statement like: if (ASSERT_DISABLED) llvm->RemoveFunctionAttr(function, LLVMStackProtectAttribute); Created attachment 232509 [details]
Native runtime library inlining in FTL for ftlopt branch
Removed the "llvm->" from FTLLowerDFGToLLVM.cpp and made the #if(ASSERT) into an if (ASSERT). Also removed additional tests from LayoutTests.
Comment on attachment 232509 [details]
Native runtime library inlining in FTL for ftlopt branch
One more bug to fix: InlineRuntimeSymbolTable.h should go do the build directory instead of being placed in the source directory.
It's OK to have InlineRuntimeSymbolTable.h in the source directory if it's checked into the repository and the build system never modifies it.
It's OK to have InlineRuntimeSymbolTable.h generated by the build system if it's in the build directory.
Right now, you're generating it with the build system, you're not checking it in (so far so good), but you're placing it into the source directory (bad).
Otherwise this is starting to look quite good!
Created attachment 232516 [details]
Native runtime library inlining in FTL for ftlopt branch
Set it such that InlineRuntimeSymbolTable now gets placed in the build directory.
Landed in http://trac.webkit.org/changeset/169628 |