Bug 208404 - [JSC] BuiltinNames' HashMap should be small
Summary: [JSC] BuiltinNames' HashMap should be small
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-28 21:58 PST by Yusuke Suzuki
Modified: 2020-03-02 10:53 PST (History)
11 users (show)

See Also:

Attachments
Patch (41.02 KB, patch)
2020-02-28 22:18 PST, Yusuke Suzuki 		mark.lam: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-02-28 21:58:22 PST 
[JSC] BuiltinNames' HashMap should be small
Comment 1 Yusuke Suzuki 2020-02-28 22:18:54 PST 
Created attachment 392049 [details]
Patch
Comment 2 Mark Lam 2020-02-28 23:51:14 PST 
Comment on attachment 392049 [details]
Patch

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

Nice work.  r=me

> Source/JavaScriptCore/ChangeLog:8
> +        This patch makes public-to-private-name-map from HashMap<RefPtr<UniquedStringImpl>, SymbolImpl*> to HashSet<String> to save half of memory.

/makes/converts/.

> Source/JavaScriptCore/builtins/BuiltinNames.cpp:66
>  #define INITIALIZE_SYMBOL_PUBLIC_TO_PRIVATE_ENTRY(name) \

nit: can we rename this to INITIALIZE_WELL_KNOWN_SYMBOL_PUBLIC_TO_PRIVATE_ENTRY?  I think this make its purpose clearer.

> Source/JavaScriptCore/builtins/BuiltinNames.h:239
>  #ifndef NDEBUG

Let's change this to #if ASSERT_ENABLED

> Source/JavaScriptCore/builtins/BuiltinNames.h:252
> +    UNUSED_PARAM(publicName);
> +    ASSERT(String(publicName.impl()) == String(privateName.impl()));

You can just use ASSERT_UNUSED(publicName, publicName.impl()) == String(privateName.impl()));

> Source/JavaScriptCore/runtime/CachedTypes.cpp:715
> +                    impl = symbol->substring(strlen("Symbol."));

Will the compiler fold strlen("Symbol.") into a constant?  If not, then maybe we should express this as:
     constexpr char symbolPrefix = "Symbol.";
     constexpr size_t symbolPrefixLength = sizeof(symbolPrefix);
     impl = symbol->substring(symbolPrefixLength);
Comment 3 Yusuke Suzuki 2020-02-28 23:57:47 PST 
Comment on attachment 392049 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch makes public-to-private-name-map from HashMap<RefPtr<UniquedStringImpl>, SymbolImpl*> to HashSet<String> to save half of memory.
> 
> /makes/converts/.

Fixed.

>> Source/JavaScriptCore/builtins/BuiltinNames.cpp:66
>>  #define INITIALIZE_SYMBOL_PUBLIC_TO_PRIVATE_ENTRY(name) \
> 
> nit: can we rename this to INITIALIZE_WELL_KNOWN_SYMBOL_PUBLIC_TO_PRIVATE_ENTRY?  I think this make its purpose clearer.

Fixed.

>> Source/JavaScriptCore/builtins/BuiltinNames.h:239
>>  #ifndef NDEBUG
> 
> Let's change this to #if ASSERT_ENABLED

Fixed.

>> Source/JavaScriptCore/builtins/BuiltinNames.h:252
>> +    ASSERT(String(publicName.impl()) == String(privateName.impl()));
> 
> You can just use ASSERT_UNUSED(publicName, publicName.impl()) == String(privateName.impl()));

Fixed.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:715
>> +                    impl = symbol->substring(strlen("Symbol."));
> 
> Will the compiler fold strlen("Symbol.") into a constant?  If not, then maybe we should express this as:
>      constexpr char symbolPrefix = "Symbol.";
>      constexpr size_t symbolPrefixLength = sizeof(symbolPrefix);
>      impl = symbol->substring(symbolPrefixLength);

Yes, typical compiler folds it into a constant.
Comment 4 Yusuke Suzuki 2020-02-29 00:04:19 PST 
Committed r257681: <https://trac.webkit.org/changeset/257681>
Comment 5 Radar WebKit Bug Importer 2020-02-29 00:05:13 PST 
<rdar://problem/59915238>
Comment 6 Yusuke Suzuki 2020-03-02 10:53:45 PST 
Committed r257719: <https://trac.webkit.org/changeset/257719>