| Summary: | Exits from exceptions shouldn't jettison code | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Saam Barati
2016-01-27 15:21:25 PST
We can also get rid of some cruft by having the ExitKind indicate what type of exception OSR exit we're talking about. looks like this will definitely help performance Benchmark report for JSRegress on il0204d-dhcp25 (MacBookPro11,3). VMs tested: "og" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r195645) "change" at /Volumes/Data/WK/a/OpenSource/WebKitBuild/Release/jsc (r195645) Collected 8 samples per benchmark/VM, with 8 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. og change getter-richards-try-catch 1296.7368+-11.5669 ^ 1206.6882+-19.2748 ^ definitely 1.0746x faster raytrace-with-empty-try-catch 7.3843+-0.0815 ? 7.4357+-0.3705 ? raytrace-with-try-catch 13.1908+-0.3264 ^ 12.1172+-0.4045 ^ definitely 1.0886x faster richards-empty-try-catch 56.1763+-0.1751 ? 56.3730+-0.2648 ? richards-try-catch 314.1001+-9.7033 ^ 288.1630+-1.3862 ^ definitely 1.0900x faster try-catch-get-by-val-cloned-arguments 8.6644+-0.2921 ? 9.0188+-0.3390 ? might be 1.0409x slower try-catch-get-by-val-direct-arguments 2.7748+-0.1006 ? 2.7958+-0.1077 ? try-catch-get-by-val-scoped-arguments 5.2259+-0.1285 ? 5.5866+-0.8754 ? might be 1.0690x slower v8-raytrace-with-empty-try-catch 63.1979+-0.8866 ? 64.5806+-2.9587 ? might be 1.0219x slower v8-raytrace-with-try-catch 83.9681+-3.3076 ^ 77.9368+-1.0780 ^ definitely 1.0774x faster <geometric> 32.8874+-0.1316 32.2882+-0.4736 might be 1.0186x faster Created attachment 270074 [details]
patch
Attachment 270074 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:135: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:120: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4]
Total errors found: 2 in 24 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270074 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270074&action=review r=me > Source/JavaScriptCore/bytecode/ExitKind.cpp:96 > +bool exitKindCausesJettison(ExitKind kind) I would call this exitKindCanJettison. "causes" made me think it always causes -- but in fact you have to meet the exit kind requirement and the count requirement before you jettison. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:10254 > + switch (exceptionType) { > + case ExceptionType::JSCall: > + case ExceptionType::GetById: > + case ExceptionType::PutById: > + exitKind = GenericUnwind; > + break; > + case ExceptionType::LazySlowPath: > + case ExceptionType::BinaryOpGenerator: > + exitKind = ExceptionCheck; > + break; I don't love these names for these concepts. I think it would nicer to go with something that distinguished "I threw the exception" from "my callee threw the exception". Maybe: LocalException ChildException OR DirectException IndirectException OR Exception CalleeException (In reply to comment #5) > Comment on attachment 270074 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270074&action=review > > r=me > > > Source/JavaScriptCore/bytecode/ExitKind.cpp:96 > > +bool exitKindCausesJettison(ExitKind kind) > > I would call this exitKindCanJettison. "causes" made me think it always > causes -- but in fact you have to meet the exit kind requirement and the > count requirement before you jettison. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:10254 > > + switch (exceptionType) { > > + case ExceptionType::JSCall: > > + case ExceptionType::GetById: > > + case ExceptionType::PutById: > > + exitKind = GenericUnwind; > > + break; > > + case ExceptionType::LazySlowPath: > > + case ExceptionType::BinaryOpGenerator: > > + exitKind = ExceptionCheck; > > + break; > > I don't love these names for these concepts. > > I think it would nicer to go with something that distinguished "I threw the > exception" from "my callee threw the exception". > > Maybe: > > LocalException > ChildException > > OR > > DirectException > IndirectException > > OR > > Exception > CalleeException I think these names make sense because it hints at our JIT code doing the exception check versus other JIT code jumping to our JIT code. A callee can throw an exception without us doing a GenericUnwind exception. I'm not sure "generic unwind" is the best name in general, and I think it might make sense to rename those one day to something more meaningful. But, given that we use genericUnwind, I think an OSRExit that has kind GenericUnwind indicates that genericUnwind found our exit machine code and that whatever exception machinery is running will jump to our GenericUnwind exit. Comment on attachment 270074 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270074&action=review >>> Source/JavaScriptCore/bytecode/ExitKind.cpp:96 >>> +bool exitKindCausesJettison(ExitKind kind) >> >> I would call this exitKindCanJettison. "causes" made me think it always causes -- but in fact you have to meet the exit kind requirement and the count requirement before you jettison. > > I think these names make sense because it hints at our JIT code doing the exception > check versus other JIT code jumping to our JIT code. A callee can throw an exception > without us doing a GenericUnwind exception. I'll go with "exitKindMayJettison" to better indicate that a threshold has to be reached. (In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 270074 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=270074&action=review > > > > r=me > > > > > Source/JavaScriptCore/bytecode/ExitKind.cpp:96 > > > +bool exitKindCausesJettison(ExitKind kind) > > > > I would call this exitKindCanJettison. "causes" made me think it always > > causes -- but in fact you have to meet the exit kind requirement and the > > count requirement before you jettison. > > > > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:10254 > > > + switch (exceptionType) { > > > + case ExceptionType::JSCall: > > > + case ExceptionType::GetById: > > > + case ExceptionType::PutById: > > > + exitKind = GenericUnwind; > > > + break; > > > + case ExceptionType::LazySlowPath: > > > + case ExceptionType::BinaryOpGenerator: > > > + exitKind = ExceptionCheck; > > > + break; > > > > I don't love these names for these concepts. > > > > I think it would nicer to go with something that distinguished "I threw the > > exception" from "my callee threw the exception". > > > > Maybe: > > > > LocalException > > ChildException I don't know what either of these things mean. > > > > OR > > > > DirectException > > IndirectException You could argue that genericUnwind is direct because it doesn't involve a check in generated code. You could argue that the excetpion check is direct because it doesn't involve calling genericUnwind. These names aren't going to work. > > > > OR > > > > Exception > > CalleeException I don't know what either of these things mean. All exceptions come from callees. > I think these names make sense because it hints at our JIT code doing the > exception > check versus other JIT code jumping to our JIT code. A callee can throw an > exception > without us doing a GenericUnwind exception. The point of these ExitKinds is to tell you if the exception involved GenericUnwind. Let's please use a name that makes this clear. If we don't like the name "GenericUnwind" then we should rename that function to something else globally. Whatever name the "genericUnwind" function has, that's the name that the ExitKind that corresponds to it should also have. landed in: http://trac.webkit.org/changeset/195831 |