Bug 153564 – Exits from exceptions shouldn't jettison code

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

We can also get rid of some cruft by having the ExitKind
indicate what type of exception OSR exit we're talking about.

[Saam Barati]

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

[Saam Barati]

Created attachment 270074 [details]
patch

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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

[Saam Barati]

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

[Saam Barati]

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.

[Saam Barati]

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.

[Filip Pizlo]

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

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/195831