Bug 152810 – FTL B3 getById() should do exceptions

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 268416 [details]
work in progress

[Filip Pizlo]

Created attachment 268439 [details]
it just might work

[Filip Pizlo]

Created attachment 268441 [details]
even nicer

Of course, I haven't tried compiling it yet.

[Filip Pizlo]

Created attachment 268444 [details]
it did things

It's starting to work.

[Filip Pizlo]

Created attachment 268471 [details]
seems to work

[Filip Pizlo]

Running tests now.

[Filip Pizlo]

Created attachment 268474 [details]
the patch

[WebKit Commit Bot]

Attachment 268474 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:58:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:67:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:68:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:69:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:88:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:104:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:105:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:106:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:107:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:108:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLB3Compile.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:357:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:360:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 15 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 268474 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268474&action=review

> Source/JavaScriptCore/dfg/DFGCommon.h:110
> -    // We're converging in a straight-forward forward flow fixpoint. This is the
> +    // We're converging in a straght-forward forward flow fixpoint. This is the

Revert.

[Filip Pizlo]

(In reply to comment #9)
> Comment on attachment 268474 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268474&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCommon.h:110
> > -    // We're converging in a straight-forward forward flow fixpoint. This is the
> > +    // We're converging in a straght-forward forward flow fixpoint. This is the
> 
> Revert.

Lol done.

[Filip Pizlo]

Comment on attachment 268474 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268474&action=review

> Source/JavaScriptCore/ftl/FTLExceptionTarget.h:49
> +    // At any time, you can 

I'll revert this.

[Saam Barati]

Comment on attachment 268474 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268474&action=review

r=me with some style comments.

> Source/JavaScriptCore/dfg/DFGCommon.h:110
> +    // We're converging in a straght-forward forward flow fixpoint. This is the

typo

> Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:46
> +Box<CCallHelpers::JumpList> ExceptionTarget::jumps(CCallHelpers& jit)

nice.

> Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:70
> +{

you could assert to verify that defaultHandler is a real thing only when isDefaultHandler is true to prevent miscreation.

> Source/JavaScriptCore/ftl/FTLExceptionTarget.h:49
> +    // At any time, you can 

remove

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:355
> +        auto exceptionHandler = state->exceptionHandler;

style: I think this is much easier to read and to determine that it's correct if you just used "Box<Label>" type here instead of auto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7226
> +        // FIXME: If this is a GetByIdFlush, we might get some performance boost if we claim that it
> +        // clobbers volatile registers late. It's not necessary for correctness, though, since the
> +        // IC code is super smart about saving registers.

might be worth opening a bug.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7242
> +                // This is the direct exit target for operation calls. It's a pointer to a JumpList.
> +                auto exceptions = exceptionHandle->scheduleExitCreation(params)->jumps(jit);

You could just remove this comment if you removed auto and wrote out the type, and also just named it something like "exceptionTargetForCallOperation".
I think that's more readable than having a comment.

> Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.h:85
> +    RefPtr<ExceptionTarget> scheduleExitCreation(const B3::StackmapGenerationParams&);
> +
> +    // Schedules the creation of an OSR exit jump destination, and ensures that it gets associated
> +    // with the handler for some callsite index.
> +    void scheduleExitCreationForUnwind(const B3::StackmapGenerationParams&, CallSiteIndex);

nice.

[Filip Pizlo]

(In reply to comment #12)
> Comment on attachment 268474 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268474&action=review
> 
> r=me with some style comments.
> 
> > Source/JavaScriptCore/dfg/DFGCommon.h:110
> > +    // We're converging in a straght-forward forward flow fixpoint. This is the
> 
> typo

Done.

> 
> > Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:46
> > +Box<CCallHelpers::JumpList> ExceptionTarget::jumps(CCallHelpers& jit)
> 
> nice.
> 
> > Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:70
> > +{
> 
> you could assert to verify that defaultHandler is a real thing only when
> isDefaultHandler is true to prevent miscreation.

Meh.  It's already a private constructor friended to PatchpointExceptionHandle.

> 
> > Source/JavaScriptCore/ftl/FTLExceptionTarget.h:49
> > +    // At any time, you can 
> 
> remove

Done.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:355
> > +        auto exceptionHandler = state->exceptionHandler;
> 
> style: I think this is much easier to read and to determine that it's
> correct if you just used "Box<Label>" type here instead of auto.

Done.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7226
> > +        // FIXME: If this is a GetByIdFlush, we might get some performance boost if we claim that it
> > +        // clobbers volatile registers late. It's not necessary for correctness, though, since the
> > +        // IC code is super smart about saving registers.
> 
> might be worth opening a bug.

Done.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7242
> > +                // This is the direct exit target for operation calls. It's a pointer to a JumpList.
> > +                auto exceptions = exceptionHandle->scheduleExitCreation(params)->jumps(jit);
> 
> You could just remove this comment if you removed auto and wrote out the
> type

I'm fine with this.

> , and also just named it something like
> "exceptionTargetForCallOperation".
> I think that's more readable than having a comment.

I prefer the "This is the direct exit target for operation calls" part as a comment. I've been having a thing for shorter variable names recently.  I think that it works well, in this case, after I write out the type. The code is now:

                // This is the direct exit target for operation calls.
                Box<CCallHelpers::JumpList> exceptions =
                    exceptionHandle->scheduleExitCreation(params)->jumps(jit);

> 
> > Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.h:85
> > +    RefPtr<ExceptionTarget> scheduleExitCreation(const B3::StackmapGenerationParams&);
> > +
> > +    // Schedules the creation of an OSR exit jump destination, and ensures that it gets associated
> > +    // with the handler for some callsite index.
> > +    void scheduleExitCreationForUnwind(const B3::StackmapGenerationParams&, CallSiteIndex);
> 
> nice.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/194716