Bug 237306 - Add a DeferTraps scope
Summary: Add a DeferTraps scope
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: InRadar
Depends on: 237409
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-28 20:05 PST by Saam Barati
Modified: 2022-03-03 11:52 PST (History)
8 users (show)

See Also:


Attachments
patch (24.64 KB, patch)
2022-02-28 21:14 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (24.64 KB, patch)
2022-02-28 21:17 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (24.86 KB, patch)
2022-02-28 21:46 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.17 KB, patch)
2022-02-28 21:48 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.13 KB, patch)
2022-03-01 10:20 PST, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing (24.97 KB, patch)
2022-03-01 18:00 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (26.06 KB, patch)
2022-03-03 10:33 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2022-02-28 20:05:37 PST
...
Comment 1 Saam Barati 2022-02-28 20:06:11 PST
<rdar://83494949>
Comment 2 Saam Barati 2022-02-28 21:14:40 PST
Created attachment 453471 [details]
patch
Comment 3 Saam Barati 2022-02-28 21:17:50 PST
Created attachment 453472 [details]
patch
Comment 4 Saam Barati 2022-02-28 21:46:07 PST
Created attachment 453474 [details]
patch
Comment 5 Saam Barati 2022-02-28 21:48:08 PST
Created attachment 453475 [details]
patch
Comment 6 Mark Lam 2022-02-28 22:03:19 PST
Comment on attachment 453475 [details]
patch

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

> Source/JavaScriptCore/interpreter/InterpreterInlines.h:99
> +        {

nit: You can remove this inner { } since this is already in the DeferTraps scope and is at the tail end.

> Source/JavaScriptCore/interpreter/InterpreterInlines.h:104
> +        // Execute the code:
> +        throwScope.release();

I think it's more logical to group these 2 lines with the execute call below.  There's no reason this needs to be here.
Comment 7 Saam Barati 2022-02-28 23:12:53 PST
Comment on attachment 453475 [details]
patch

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

>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:99
>> +        {
> 
> nit: You can remove this inner { } since this is already in the DeferTraps scope and is at the tail end.

I think it’s nice to keep this isolated as is. It shows exactly the region of code we’re asserting against

>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:104
>> +        throwScope.release();
> 
> I think it's more logical to group these 2 lines with the execute call below.  There's no reason this needs to be here.

If we make release() ever service traps we wouldn’t want to put it below. We want maximal things in this scope as long as we don’t recurse. Maybe we won’t ever make release() do that, but I see no downside to keeping it in here. An alternative I considered is to add a release() style function call to DeferTraps, and to use it in these types of situations so we have fewer scopes, but it seems a bit overkill
Comment 8 Mark Lam 2022-02-28 23:17:37 PST
Comment on attachment 453475 [details]
patch

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

>>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:99
>>> +        {
>> 
>> nit: You can remove this inner { } since this is already in the DeferTraps scope and is at the tail end.
> 
> I think it’s nice to keep this isolated as is. It shows exactly the region of code we’re asserting against

ok, SGTM.

>>> Source/JavaScriptCore/interpreter/InterpreterInlines.h:104
>>> +        throwScope.release();
>> 
>> I think it's more logical to group these 2 lines with the execute call below.  There's no reason this needs to be here.
> 
> If we make release() ever service traps we wouldn’t want to put it below. We want maximal things in this scope as long as we don’t recurse. Maybe we won’t ever make release() do that, but I see no downside to keeping it in here. An alternative I considered is to add a release() style function call to DeferTraps, and to use it in these types of situations so we have fewer scopes, but it seems a bit overkill

ok.
Comment 9 Saam Barati 2022-03-01 10:20:12 PST
Created attachment 453509 [details]
patch

rebased
Comment 10 Mark Lam 2022-03-01 14:55:38 PST
Comment on attachment 453509 [details]
patch

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:955
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1020
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1311
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1407
>      // Execute the code:
>      throwScope.release();

You didn't put this ThrowScope::release() in the DeferScope unlike in `Interpreter::execute(CallFrameClosure& closure)` below.  The same is true for all Interpreter execute functions above.  Realistically, I don't think we will or should ever handle traps in ThrowScope::release() because on release builds, we rely on it being a no-op.  I still think that it is the better idiom to keep the ThrowScope::release() close to the statement that the release applies to i.e. the execute.  Also, the code reads more naturally this way.  Regardless, let's make it consistent every where.

> Source/JavaScriptCore/interpreter/InterpreterInlines.h:91
> +        DeferTraps deferTraps(vm); // We can't jettison this code if we're about to run it.

nit: can you add a newline after this decl?
Comment 11 Saam Barati 2022-03-01 18:00:30 PST
Created attachment 453557 [details]
patch for landing
Comment 12 EWS 2022-03-01 19:43:53 PST
Committed r290717 (247964@main): <https://commits.webkit.org/247964@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453557 [details].
Comment 13 WebKit Commit Bot 2022-03-02 23:06:12 PST
Re-opened since this is blocked by bug 237409
Comment 14 Saam Barati 2022-03-03 10:33:15 PST
Created attachment 453759 [details]
patch
Comment 15 Mark Lam 2022-03-03 10:36:53 PST
Comment on attachment 453759 [details]
patch

r=me
Comment 16 EWS 2022-03-03 11:52:40 PST
Committed r290788 (248030@main): <https://commits.webkit.org/248030@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453759 [details].