Bug 226909

Summary: Use DOMConstructor array instead of HashMap since window constructor property access is critical
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, annulen, beidson, benjamin, calvaris, cdumez, darin, ews-watchlist, fpizlo, gyuyoung.kim, jasemabeed114, jsbell, ryuan.choi, sam, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch fpizlo: review+

Yusuke Suzuki
Reported 2021-06-10 19:16:35 PDT
Use DOMConstructor array instead of HashMap since window constructor property access is critical
Attachments
Patch (137.53 KB, patch)
2021-06-10 19:20 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (137.53 KB, patch)
2021-06-10 19:53 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (137.55 KB, patch)
2021-06-10 19:58 PDT, Yusuke Suzuki 		fpizlo: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2021-06-10 19:20:29 PDT
Created attachment 431168 [details] Patch
Yusuke Suzuki
Comment 2 2021-06-10 19:53:36 PDT
Created attachment 431170 [details] Patch
Chris Dumez
Comment 3 2021-06-10 19:56:12 PDT
Comment on attachment 431170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431170&action=review > Source/WebCore/ChangeLog:12 > + preprocess-idls.pl collects all constructors and assign DOMConstructor::XXX enum to each constructor. And it also counts What about constructors added at runtime (e.g. the ones marked as [EnabledAtRuntime] or [EnabledBySetting]) ?
Yusuke Suzuki
Comment 4 2021-06-10 19:58:08 PDT
Comment on attachment 431170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431170&action=review >> Source/WebCore/ChangeLog:12 >> + preprocess-idls.pl collects all constructors and assign DOMConstructor::XXX enum to each constructor. And it also counts > > What about constructors added at runtime (e.g. the ones marked as [EnabledAtRuntime] or [EnabledBySetting]) ? Regardless of whether it is defined at runtime, we have ID for that constructor.
Yusuke Suzuki
Comment 5 2021-06-10 19:58:28 PDT
Created attachment 431171 [details] Patch
Yusuke Suzuki
Comment 6 2021-06-11 18:20:47 PDT
Committed r278802 (238759@main): <https://commits.webkit.org/238759@main>
Radar WebKit Bug Importer
Comment 7 2021-06-11 18:21:21 PDT
<rdar://problem/79226523>
Darin Adler
Comment 8 2021-06-13 12:08:03 PDT
Comment on attachment 431171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431171&action=review > Source/WebCore/bindings/js/JSDOMGlobalObject.h:127 > + std::unique_ptr<DOMConstructors> m_constructors; Can this be std::unique_ptr<const DOMConstructors>? If not, why not?
Sam Weinig
Comment 9 2021-06-14 16:46:09 PDT
Nice. Should we do this for the DOM Structures map too?
Yusuke Suzuki
Comment 10 2021-07-30 16:23:22 PDT
Comment on attachment 431171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431171&action=review >> Source/WebCore/bindings/js/JSDOMGlobalObject.h:127 >> + std::unique_ptr<DOMConstructors> m_constructors; > > Can this be std::unique_ptr<const DOMConstructors>? If not, why not? Because DOMConstructors will be modified :)
Yusuke Suzuki
Comment 11 2021-07-30 16:24:08 PDT
(In reply to Sam Weinig from comment #9) > Nice. Should we do this for the DOM Structures map too? Possibly yes :) Or, we could extend it to using LazyClassStructure (which can hold construtors and structures)
Note You need to log in before you can comment on or make changes to this bug.