[JSC] BuiltinExecutables should behave like a WeakSet instead of generic WeakHandleOwner for memory footprint

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Weak<> is not cheap feature in terms of memory.

[Yusuke Suzuki]

Created attachment 364111 [details]
Patch

[Robin Morisset]

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 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 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]

(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 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]

Committed r242722: <https://trac.webkit.org/changeset/242722>

[Radar WebKit Bug Importer]

<rdar://problem/48775996>