Bug 153564 - Exits from exceptions shouldn't jettison code
Summary: Exits from exceptions shouldn't jettison code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-27 15:21 PST by Saam Barati
Modified: 2016-01-29 11:46 PST (History)
11 users (show)

See Also:


Attachments
patch (77.50 KB, patch)
2016-01-27 18:52 PST, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-01-27 15:21:25 PST
...
Comment 1 Saam Barati 2016-01-27 17:59: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.
Comment 2 Saam Barati 2016-01-27 18:10:47 PST
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
Comment 3 Saam Barati 2016-01-27 18:52:13 PST
Created attachment 270074 [details]
patch
Comment 4 WebKit Commit Bot 2016-01-27 19:07:30 PST
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 5 Geoffrey Garen 2016-01-28 10:37:17 PST
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
Comment 6 Saam Barati 2016-01-29 11:31:41 PST
(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.
Comment 7 Saam Barati 2016-01-29 11:33:27 PST
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 8 Saam Barati 2016-01-29 11:34:33 PST
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.
Comment 9 Filip Pizlo 2016-01-29 11:36:22 PST
(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.
Comment 10 Saam Barati 2016-01-29 11:46:33 PST
landed in:
http://trac.webkit.org/changeset/195831