Bug 208404 – [JSC] BuiltinNames' HashMap should be small

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] BuiltinNames' HashMap should be small

[Yusuke Suzuki]

Created attachment 392049 [details]
Patch

[Mark Lam]

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);

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r257681: <https://trac.webkit.org/changeset/257681>

[Radar WebKit Bug Importer]

<rdar://problem/59915238>

[Yusuke Suzuki]

Committed r257719: <https://trac.webkit.org/changeset/257719>