| Summary: | [JSC] BuiltinNames' HashMap should be small | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Yusuke Suzuki
2020-02-28 21:58:22 PST
Created attachment 392049 [details]
Patch
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 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. Committed r257681: <https://trac.webkit.org/changeset/257681> Committed r257719: <https://trac.webkit.org/changeset/257719> |