Bug 128625 - Add fast mapping from StringImpl to JSString
Summary: Add fast mapping from StringImpl to JSString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-11 13:57 PST by Gavin Barraclough
Modified: 2014-02-18 19:16 PST (History)
12 users (show)

See Also:


Attachments
Probably unsafe WRT static strings, workers, isolated worlds & maybe JSC API too. But basic browsing seems to work. (11.06 KB, patch)
2014-02-11 14:49 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix (16.42 KB, patch)
2014-02-17 13:43 PST, Gavin Barraclough
ggaren: review+
Details | Formatted Diff | Diff
Already reviewed, just for EWS & giggles. (16.34 KB, patch)
2014-02-17 17:04 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2014-02-11 13:57:12 PST
.
Comment 1 Gavin Barraclough 2014-02-11 14:49:41 PST
Created attachment 223904 [details]
Probably unsafe WRT static strings, workers, isolated worlds & maybe JSC API too. But basic browsing seems to work.
Comment 2 WebKit Commit Bot 2014-02-11 14:52:04 PST
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.
Comment 3 Gavin Barraclough 2014-02-17 13:43:51 PST
Created attachment 224417 [details]
Fix
Comment 4 WebKit Commit Bot 2014-02-17 13:44:30 PST
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.
Comment 5 Andreas Kling 2014-02-17 14:43:16 PST
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.
Comment 6 Geoffrey Garen 2014-02-17 14:59:35 PST
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.
Comment 7 Gavin Barraclough 2014-02-17 17:04:14 PST
Created attachment 224447 [details]
Already reviewed, just for EWS & giggles.
Comment 8 WebKit Commit Bot 2014-02-17 18:06:11 PST
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.
Comment 9 Gavin Barraclough 2014-02-18 19:16:36 PST
Transmitting file data ..............
Committed revision 164347.