| Summary: | Add fast mapping from StringImpl to JSString | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gavin Barraclough <barraclough> | ||||||||
| Component: | WebCore JavaScript | Assignee: | Gavin Barraclough <barraclough> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | andersca, benjamin, cdumez, cgarcia, cmarcelo, commit-queue, darin, ggaren, kling, koivisto, mhahnenberg, rniwa | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Gavin Barraclough
2014-02-11 13:57:12 PST
Created attachment 223904 [details]
Probably unsafe WRT static strings, workers, isolated worlds & maybe JSC API too. But basic browsing seems to work.
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.
Created attachment 224417 [details]
Fix
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 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 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. Created attachment 224447 [details]
Already reviewed, just for EWS & giggles.
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.
Transmitting file data .............. Committed revision 164347. |