Bug 185803

Summary: [DFG] InById should be converted to MatchStructure
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, ticaiolima, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 185682, 185695    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch keith_miller: review+

Yusuke Suzuki
Reported 2018-05-19 12:42:04 PDT
...
Attachments
Patch (25.38 KB, patch)
2018-05-19 15:24 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (27.86 KB, patch)
2018-05-19 16:36 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (39.86 KB, patch)
2018-05-19 17:26 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (41.59 KB, patch)
2018-05-20 05:01 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (38.12 KB, patch)
2018-05-30 07:36 PDT, Yusuke Suzuki 		keith_miller: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2018-05-19 15:24:47 PDT
Created attachment 340787 [details] Patch
Yusuke Suzuki
Comment 2 2018-05-19 16:36:25 PDT
Created attachment 340788 [details] Patch
Yusuke Suzuki
Comment 3 2018-05-19 17:26:24 PDT
Created attachment 340789 [details] Patch
Yusuke Suzuki
Comment 4 2018-05-20 05:01:32 PDT
Created attachment 340793 [details] Patch
Caio Lima
Comment 5 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.
Yusuke Suzuki
Comment 6 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.
Yusuke Suzuki
Comment 7 2018-05-30 07:36:38 PDT
Created attachment 341571 [details] Patch
Yusuke Suzuki
Comment 8 2018-05-31 18:26:38 PDT
Ping?
Keith Miller
Comment 9 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)
Yusuke Suzuki
Comment 10 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.
Yusuke Suzuki
Comment 11 2018-06-01 11:40:02 PDT
Committed r232400: <https://trac.webkit.org/changeset/232400>
Radar WebKit Bug Importer
Comment 12 2018-06-01 11:46:27 PDT
<rdar://problem/40728977>
Note You need to log in before you can comment on or make changes to this bug.