| Summary: | Cache toString results for CoW arrays | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, darin, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, realdawei, rmorisset, ryanhaddad, ticaiolima, webkit-bug-importer, wenson_hsieh, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | Safari Technology Preview | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Saam Barati
2018-05-31 13:46:31 PDT
Created attachment 341701 [details]
patch
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. 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 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. Created attachment 341706 [details]
patch for landing
Created attachment 341707 [details]
patch for landing
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 (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. Comment on attachment 341707 [details] patch for landing Clearing flags on attachment: 341707 Committed r232385: <https://trac.webkit.org/changeset/232385> All reviewed patches have been landed. Closing bug. 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 (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. (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 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. 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. 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. (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... |