Bug 186160 – Cache toString results for CoW arrays

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 341701 [details]
patch

[Keith Miller]

Comment on attachment 341701 [details]
patch

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

r=me with comments.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
> +static inline bool canUseFastJoin(const JSObject* thisObject)

Why both static and inline? that's the same as just inline...

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:416
> +template<typename T> static inline bool containsHole(T* data, unsigned length)

Newline between the template and the rest of the declaration.

[Saam Barati]

Comment on attachment 341701 [details]
patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
>> +static inline bool canUseFastJoin(const JSObject* thisObject)
> 
> Why both static and inline? that's the same as just inline...

because that's how this code was before. I'll move to just inline.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:417
> +{

ditto and ditto

[Mark Lam]

Comment on attachment 341701 [details]
patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
>> +static inline bool canUseFastJoin(const JSObject* thisObject)
> 
> Why both static and inline? that's the same as just inline...

"static" does not require a prototype.  Just "inline" does.

[Saam Barati]

Created attachment 341706 [details]
patch for landing

[Saam Barati]

Created attachment 341707 [details]
patch for landing

[Keith Miller]

Comment on attachment 341701 [details]
patch

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

>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
>>>> +static inline bool canUseFastJoin(const JSObject* thisObject)
>>> 
>>> Why both static and inline? that's the same as just inline...
>> 
>> because that's how this code was before. I'll move to just inline.
> 
> "static" does not require a prototype.  Just "inline" does.

It totes doesn't need a prototype with just "inline". Look at hasIndexedProperties in IndexingType.h

[Mark Lam]

(In reply to Keith Miller from comment #7)
> Comment on attachment 341701 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341701&action=review
> 
> >>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
> >>>> +static inline bool canUseFastJoin(const JSObject* thisObject)
> >>> 
> >>> Why both static and inline? that's the same as just inline...
> >> 
> >> because that's how this code was before. I'll move to just inline.
> > 
> > "static" does not require a prototype.  Just "inline" does.
> 
> It totes doesn't need a prototype with just "inline". Look at
> hasIndexedProperties in IndexingType.h

I stand corrected.

[WebKit Commit Bot]

Comment on attachment 341707 [details]
patch for landing

Clearing flags on attachment: 341707

Committed r232385: <https://trac.webkit.org/changeset/232385>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40709425>

[Dawei Fenton (:realdawei)]

Looks like we have some LLINT CLoop build issues after this latest patch

https://build.webkit.org/builders/Apple%20High%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/6306/steps/compile-webkit/logs/stdio

./runtime/ArrayPrototype.cpp:576:34: error: incomplete type 'JSC::JSImmutableButterfly' named in nested name specifier
./runtime/ArrayPrototype.cpp:586:20: error: incomplete type 'JSC::JSImmutableButterfly' named in nested name specifier

[Wenson Hsieh]

(In reply to David Fenton from comment #12)
> Looks like we have some LLINT CLoop build issues after this latest patch
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/6306/steps/
> compile-webkit/logs/stdio
> 
> ./runtime/ArrayPrototype.cpp:576:34: error: incomplete type
> 'JSC::JSImmutableButterfly' named in nested name specifier
> ./runtime/ArrayPrototype.cpp:586:20: error: incomplete type
> 'JSC::JSImmutableButterfly' named in nested name specifier

I just ran into this and filed https://bugs.webkit.org/show_bug.cgi?id=186203.

[Saam Barati]

(In reply to Wenson Hsieh from comment #13)
> (In reply to David Fenton from comment #12)
> > Looks like we have some LLINT CLoop build issues after this latest patch
> > 
> > https://build.webkit.org/builders/
> > Apple%20High%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/6306/steps/
> > compile-webkit/logs/stdio
> > 
> > ./runtime/ArrayPrototype.cpp:576:34: error: incomplete type
> > 'JSC::JSImmutableButterfly' named in nested name specifier
> > ./runtime/ArrayPrototype.cpp:586:20: error: incomplete type
> > 'JSC::JSImmutableButterfly' named in nested name specifier
> 
> I just ran into this and filed
> https://bugs.webkit.org/show_bug.cgi?id=186203.

Thanks for fixing this

[Darin Adler]

Comment on attachment 341701 [details]
patch

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

>>>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
>>>>>> +static inline bool canUseFastJoin(const JSObject* thisObject)
>>>>> 
>>>>> Why both static and inline? that's the same as just inline...
>>>> 
>>>> because that's how this code was before. I'll move to just inline.
>>> 
>>> "static" does not require a prototype.  Just "inline" does.
>> 
>> It totes doesn't need a prototype with just "inline". Look at hasIndexedProperties in IndexingType.h
> 
> I stand corrected.

It’s true that static and inline is similar to just inline, but it’s not *quite* the same. Maybe no differences that we care about here, but there is at least one difference that I know of:

If there is a global variable inside the function (confusingly, you do that with the same keyword, static) and the function is marked static, then each copy of the function in each translation unit has its own distinct global variable with a separate value. If the function is not marked static and is marked only inline, then all copies of the function in all translation units will share a single global variable with a single value.

[Darin Adler]

Comment on attachment 341701 [details]
patch

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

>>>>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
>>>>>>> +static inline bool canUseFastJoin(const JSObject* thisObject)
>>>>>> 
>>>>>> Why both static and inline? that's the same as just inline...
>>>>> 
>>>>> because that's how this code was before. I'll move to just inline.
>>>> 
>>>> "static" does not require a prototype.  Just "inline" does.
>>> 
>>> It totes doesn't need a prototype with just "inline". Look at hasIndexedProperties in IndexingType.h
>> 
>> I stand corrected.
> 
> It’s true that static and inline is similar to just inline, but it’s not *quite* the same. Maybe no differences that we care about here, but there is at least one difference that I know of:
> 
> If there is a global variable inside the function (confusingly, you do that with the same keyword, static) and the function is marked static, then each copy of the function in each translation unit has its own distinct global variable with a separate value. If the function is not marked static and is marked only inline, then all copies of the function in all translation units will share a single global variable with a single value.

I thought of a second difference:

If we mark a function static, and then don’t use it, we will get an unused function warning. If we don’t mark it static, then we don’t get the warning. That seems like a good reason to mark functions in .cpp files static: We get the compiler’s help in noticing dead code.

[Darin Adler]

Comment on attachment 341701 [details]
patch

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

>>>>>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
>>>>>>>> +static inline bool canUseFastJoin(const JSObject* thisObject)
>>>>>>> 
>>>>>>> Why both static and inline? that's the same as just inline...
>>>>>> 
>>>>>> because that's how this code was before. I'll move to just inline.
>>>>> 
>>>>> "static" does not require a prototype.  Just "inline" does.
>>>> 
>>>> It totes doesn't need a prototype with just "inline". Look at hasIndexedProperties in IndexingType.h
>>> 
>>> I stand corrected.
>> 
>> It’s true that static and inline is similar to just inline, but it’s not *quite* the same. Maybe no differences that we care about here, but there is at least one difference that I know of:
>> 
>> If there is a global variable inside the function (confusingly, you do that with the same keyword, static) and the function is marked static, then each copy of the function in each translation unit has its own distinct global variable with a separate value. If the function is not marked static and is marked only inline, then all copies of the function in all translation units will share a single global variable with a single value.
> 
> I thought of a second difference:
> 
> If we mark a function static, and then don’t use it, we will get an unused function warning. If we don’t mark it static, then we don’t get the warning. That seems like a good reason to mark functions in .cpp files static: We get the compiler’s help in noticing dead code.

Third difference, one that we must not take advantage of because of our "compiling more than one file at a time" optimization, but it is a difference:

Functions marked static have what’s called "internal linkage". This means that two functions with the same signature don’t conflict with each other. While this can be confusing, two identically named functions, it might also be valuable to know there won’t be a conflict. Functions that are not marked static, even ones marked inline, have "external linkage". That means that two functions with the same signature may conflict. Even a function marked "inline" might get a non-inlined copy that can conflict. Using static guarantees there is no error due to the conflict at link time.

[Keith Miller]

(In reply to Darin Adler from comment #15)
> Comment on attachment 341701 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341701&action=review
> 
> >>>>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:388
> >>>>>> +static inline bool canUseFastJoin(const JSObject* thisObject)
> >>>>> 
> >>>>> Why both static and inline? that's the same as just inline...
> >>>> 
> >>>> because that's how this code was before. I'll move to just inline.
> >>> 
> >>> "static" does not require a prototype.  Just "inline" does.
> >> 
> >> It totes doesn't need a prototype with just "inline". Look at hasIndexedProperties in IndexingType.h
> > 
> > I stand corrected.
> 
> It’s true that static and inline is similar to just inline, but it’s not
> *quite* the same. Maybe no differences that we care about here, but there is
> at least one difference that I know of:
> 
> If there is a global variable inside the function (confusingly, you do that
> with the same keyword, static) and the function is marked static, then each
> copy of the function in each translation unit has its own distinct global
> variable with a separate value. If the function is not marked static and is
> marked only inline, then all copies of the function in all translation units
> will share a single global variable with a single value.

What?!? I did not know this... That kinda makes sense but is also pretty insane...