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+

Description Yusuke Suzuki 2021-06-10 19:16:35 PDT 
Use DOMConstructor array instead of HashMap since window constructor property access is critical
Comment 1 Yusuke Suzuki 2021-06-10 19:20:29 PDT 
Created attachment 431168 [details]
Patch
Comment 2 Yusuke Suzuki 2021-06-10 19:53:36 PDT 
Created attachment 431170 [details]
Patch
Comment 3 Chris Dumez 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]) ?
Comment 4 Yusuke Suzuki 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.
Comment 5 Yusuke Suzuki 2021-06-10 19:58:28 PDT 
Created attachment 431171 [details]
Patch
Comment 6 Yusuke Suzuki 2021-06-11 18:20:47 PDT 
Committed r278802 (238759@main): <https://commits.webkit.org/238759@main>
Comment 7 Radar WebKit Bug Importer 2021-06-11 18:21:21 PDT 
<rdar://problem/79226523>
Comment 8 Darin Adler 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?
Comment 9 Sam Weinig 2021-06-14 16:46:09 PDT 
Nice. Should we do this for the DOM Structures map too?
Comment 10 Yusuke Suzuki 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 :)
Comment 11 Yusuke Suzuki 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)