Bug 128625 – Add fast mapping from StringImpl to JSString

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Gavin Barraclough]

.

[Gavin Barraclough]

Created attachment 223904 [details]
Probably unsafe WRT static strings, workers, isolated worlds & maybe JSC API too. But basic browsing seems to work.

[WebKit Commit Bot]

Attachment 223904 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSString.h:96:  The parameter name "impl" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 224417 [details]
Fix

[WebKit Commit Bot]

Attachment 224417 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.h:58:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 224417 [details]
Fix

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

I'm not JSC ninja enough to do final review, so here's some minor comments. (This looks really awesome to me btw.)

> Source/JavaScriptCore/runtime/JSString.cpp:305
> +    impl->setWeakJSString(0);

0 -> nullptr

> Source/JavaScriptCore/runtime/JSString.h:66
> +        class WeakOwner : public WeakHandleOwner {

final

> Source/JavaScriptCore/runtime/JSString.h:68
> +            virtual void finalize(Handle<Unknown>, void* context);

override

> Source/JavaScriptCore/runtime/JSString.h:413
> +    inline JSString* jsStringWithWeakOwner(VM* vm, const String& s)

's' is kind of an unWebKitty variable name but I guess it's okay in this context.

> Source/JavaScriptCore/runtime/JSString.h:418
> +        // If this vm is not allowed weakly own strings just call jsString.

allowed *to*

> Source/JavaScriptCore/runtime/JSString.h:427
> +            impl->setWeakJSString(0);

0 -> nullptr

> Source/JavaScriptCore/runtime/VM.cpp:173
> +    , jsStringWeakOwner(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:169
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:187
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:205
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:219
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:233
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:247
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:260
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:274
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:288
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:306
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WTF/wtf/text/StringImpl.h:326
> +        , m_weakJSString(0)

0 -> nullptr

> Source/WebCore/bindings/js/DOMWrapperWorld.h:-36
> -typedef JSC::WeakGCMap<StringImpl*, JSC::JSString, PtrHash<StringImpl*>> JSStringCache;

I guess we don't need the WeakGCMap.h include after this.

[Geoffrey Garen]

Comment on attachment 224417 [details]
Fix

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

r=me

> Source/JavaScriptCore/runtime/VM.h:227
> +        WeakHandleOwner* jsStringWeakOwner;

This should be OwnPtr, even though we currently only use this for the leaked VM, to avoid too many cross-cutting memory management assumptions.

[Gavin Barraclough]

Created attachment 224447 [details]
Already reviewed, just for EWS & giggles.

[WebKit Commit Bot]

Attachment 224447 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringImpl.h:58:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Transmitting file data ..............
Committed revision 164347.