WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
195508
[JSC] BuiltinExecutables should behave like a WeakSet instead of generic WeakHandleOwner for memory footprint
https://bugs.webkit.org/show_bug.cgi?id=195508
Summary
[JSC] BuiltinExecutables should behave like a WeakSet instead of generic Weak...
Yusuke Suzuki
Reported
2019-03-08 18:45:13 PST
Weak<> is not cheap feature in terms of memory.
Attachments
Patch
(7.56 KB, patch)
2019-03-08 20:43 PST
,
Yusuke Suzuki
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2019-03-08 20:43:03 PST
Created
attachment 364111
[details]
Patch
Robin Morisset
Comment 2
2019-03-08 20:55:37 PST
Comment on
attachment 364111
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=364111&action=review
I like this change a lot. I would r=me it but I am not yet an official reviewer.
> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:288 > +#undef DEFINE_BUILTIN_EXECUTABLES
So we were undef-ing the wrong macro?! Good catch.
Darin Adler
Comment 3
2019-03-10 13:45:43 PDT
Comment on
attachment 364111
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=364111&action=review
> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 > + for (UnlinkedFunctionExecutable*& executable : m_executables) {
I know other people don’t agree, but I really prefer "auto&" in a case like this.
> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:273 > + return SourceCode(m_combinedSourceProvider.copyRef(), s_##name - s_JSCCombinedCode, (s_##name - s_JSCCombinedCode) + length, 1, 1);\
The SourceCode { } syntax might be better to use here, giving slightly stricter type checking. And possibly just { } without the explicit SourceCode.
> Source/JavaScriptCore/builtins/BuiltinExecutables.h:40 > +#define PUT_BUILTIN_CODE_ID(name, functionName, overriddenName, length) name,
Do we want to also include an #undef for this one? Maybe a shorter name for the macro too, like BUILTIN_NAME_ONLY.
> Source/JavaScriptCore/builtins/BuiltinExecutables.h:64 > + void finalizeUnconditionally(); > +private:
Typically we leave a blank line before the private section.
Keith Miller
Comment 4
2019-03-10 17:13:09 PDT
Comment on
attachment 364111
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=364111&action=review
>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 >> + for (UnlinkedFunctionExecutable*& executable : m_executables) { > > I know other people don’t agree, but I really prefer "auto&" in a case like this.
In think case, I would personally use the full type as Yusuke did. I like using auto when the type isn't completely clear from the context. Here, it's not clear that the executables are UnlinkedFunctionExecutables rather than say ScriptExecutable. In a hypothetical world where indexing didn't take forever, I'd agree that the type shouldn't be needed. I should be able to ask my editor to get that information from the compiler. Alas, that world doesn't exist yet... :(
Darin Adler
Comment 5
2019-03-10 18:15:07 PDT
(In reply to Keith Miller from
comment #4
)
> Comment on
attachment 364111
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=364111&action=review
> > >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 > >> + for (UnlinkedFunctionExecutable*& executable : m_executables) { > > > > I know other people don’t agree, but I really prefer "auto&" in a case like this. > > In think case, I would personally use the full type as Yusuke did. I like > using auto when the type isn't completely clear from the context. Here, it's > not clear that the executables are UnlinkedFunctionExecutables rather than > say ScriptExecutable. > > In a hypothetical world where indexing didn't take forever, I'd agree that > the type shouldn't be needed. I should be able to ask my editor to get that > information from the compiler. Alas, that world doesn't exist yet... :(
I’m going to try to not rehash the argument, but I will point out, once again, that we seem perfectly able to call functions and use the function results to call a member function without naming the type of each function result. I don’t think that a for loop value is different in this respect, unless the logic inside the loop is complex. It’s not as simple for me as "can you look up the type easily?".
Yusuke Suzuki
Comment 6
2019-03-11 11:43:44 PDT
Comment on
attachment 364111
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=364111&action=review
>>>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 >>>> + for (UnlinkedFunctionExecutable*& executable : m_executables) { >>> >>> I know other people don’t agree, but I really prefer "auto&" in a case like this. >> >> In think case, I would personally use the full type as Yusuke did. I like using auto when the type isn't completely clear from the context. Here, it's not clear that the executables are UnlinkedFunctionExecutables rather than say ScriptExecutable. >> >> In a hypothetical world where indexing didn't take forever, I'd agree that the type shouldn't be needed. I should be able to ask my editor to get that information from the compiler. Alas, that world doesn't exist yet... :( > > I’m going to try to not rehash the argument, but I will point out, once again, that we seem perfectly able to call functions and use the function results to call a member function without naming the type of each function result. I don’t think that a for loop value is different in this respect, unless the logic inside the loop is complex. > > It’s not as simple for me as "can you look up the type easily?".
Personally, I like writing full-types as long as the type is meaningful (like, I don't like writing std::vector<int>::const_iterator etc.). So, maybe, `m_unlinkedExecutables` and `unlinkedExecutable` variable names would allow us to use `auto*&`, while keeping its type clear :) Fixed.
>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:273 >> + return SourceCode(m_combinedSourceProvider.copyRef(), s_##name - s_JSCCombinedCode, (s_##name - s_JSCCombinedCode) + length, 1, 1);\ > > The SourceCode { } syntax might be better to use here, giving slightly stricter type checking. And possibly just { } without the explicit SourceCode.
Fixed.
>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:288 >> +#undef DEFINE_BUILTIN_EXECUTABLES > > So we were undef-ing the wrong macro?! Good catch.
:)
>> Source/JavaScriptCore/builtins/BuiltinExecutables.h:40 >> +#define PUT_BUILTIN_CODE_ID(name, functionName, overriddenName, length) name, > > Do we want to also include an #undef for this one? Maybe a shorter name for the macro too, like BUILTIN_NAME_ONLY.
Yeah, fixed and changed.
>> Source/JavaScriptCore/builtins/BuiltinExecutables.h:64 >> +private: > > Typically we leave a blank line before the private section.
Fixed.
Yusuke Suzuki
Comment 7
2019-03-11 11:59:30 PDT
Committed
r242722
: <
https://trac.webkit.org/changeset/242722
>
Radar WebKit Bug Importer
Comment 8
2019-03-11 12:01:13 PDT
<
rdar://problem/48775996
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug