Bug 185803 - [DFG] InById should be converted to MatchStructure
Summary: [DFG] InById should be converted to MatchStructure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 185682 185695
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-19 12:42 PDT by Yusuke Suzuki
Modified: 2018-06-01 11:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (25.38 KB, patch)
2018-05-19 15:24 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.86 KB, patch)
2018-05-19 16:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (39.86 KB, patch)
2018-05-19 17:26 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (41.59 KB, patch)
2018-05-20 05:01 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (38.12 KB, patch)
2018-05-30 07:36 PDT, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-05-19 12:42:04 PDT
...
Comment 1 Yusuke Suzuki 2018-05-19 15:24:47 PDT
Created attachment 340787 [details]
Patch
Comment 2 Yusuke Suzuki 2018-05-19 16:36:25 PDT
Created attachment 340788 [details]
Patch
Comment 3 Yusuke Suzuki 2018-05-19 17:26:24 PDT
Created attachment 340789 [details]
Patch
Comment 4 Yusuke Suzuki 2018-05-20 05:01:32 PDT
Created attachment 340793 [details]
Patch
Comment 5 Caio Lima 2018-05-22 05:16:35 PDT
Comment on attachment 340793 [details]
Patch

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

I think it is good add tests considering proxy cases and check if we are not breaking it. Also, Is there any kind of program we could add on microbenchmark to check performance?

> Source/JavaScriptCore/bytecode/InByIdStatus.cpp:170
> +            ComplexGetStatus complexGetStatus = ComplexGetStatus::computeFor(structure, access.conditionSet(), uid);

Maybe it is time to rename this class to "ComplexPropertyAccessStatus" or something related? Current name is not reflecting the real usage of it.
Comment 6 Yusuke Suzuki 2018-05-30 07:35:30 PDT
Comment on attachment 340793 [details]
Patch

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

Sounds nice, I've added a micorbenchmarks.

>> Source/JavaScriptCore/bytecode/InByIdStatus.cpp:170
>> +            ComplexGetStatus complexGetStatus = ComplexGetStatus::computeFor(structure, access.conditionSet(), uid);
> 
> Maybe it is time to rename this class to "ComplexPropertyAccessStatus" or something related? Current name is not reflecting the real usage of it.

I would like to make it a separate patch since it involves many changes.
Comment 7 Yusuke Suzuki 2018-05-30 07:36:38 PDT
Created attachment 341571 [details]
Patch
Comment 8 Yusuke Suzuki 2018-05-31 18:26:38 PDT
Ping?
Comment 9 Keith Miller 2018-06-01 11:16:22 PDT
Comment on attachment 341571 [details]
Patch

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

r=me.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6458
> +                        data->variants.append(matchVariant);

Nit: Not that it matters now but we should probably WTFMove(matchVariant)
Comment 10 Yusuke Suzuki 2018-06-01 11:37:54 PDT
Comment on attachment 341571 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6458
>> +                        data->variants.append(matchVariant);
> 
> Nit: Not that it matters now but we should probably WTFMove(matchVariant)

Nice, fixed.
Comment 11 Yusuke Suzuki 2018-06-01 11:40:02 PDT
Committed r232400: <https://trac.webkit.org/changeset/232400>
Comment 12 Radar WebKit Bug Importer 2018-06-01 11:46:27 PDT
<rdar://problem/40728977>