Bug 131923

Summary: Speed up jsStringWithCache() through WeakGCMap inlining.
Product: WebKit Reporter: Andreas Kling <kling>
Component: BindingsAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, commit-queue, kling
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Andreas Kling 2014-04-20 20:28:39 PDT
Speed up jsStringWithCache() through WeakGCMap inlining.
Comment 1 Andreas Kling 2014-04-20 20:33:16 PDT
Created attachment 229780 [details]
Patch
Comment 2 Darin Adler 2014-04-20 20:40:04 PDT
Comment on attachment 229780 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.cpp:66
> +JSC::JSValue jsStringWithCache(JSC::ExecState* exec, const String& s)

rename s to string?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:81
> +    JSStringCache& stringCache = currentWorld(exec).m_stringCache;
> +    JSStringCache::AddResult addResult = stringCache.add(stringImpl, nullptr);

I suggest using auto. I also suggest getting rid of the stringCache local variable.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:83
> +        addResult.iterator->value = JSC::jsString(exec, String(stringImpl));

Seems a shame to do String(stringImpl) here instead of just using "s"
Comment 3 Andreas Kling 2014-04-20 20:54:40 PDT
(In reply to comment #2)
> (From update of attachment 229780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229780&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:66
> > +JSC::JSValue jsStringWithCache(JSC::ExecState* exec, const String& s)
> 
> rename s to string?
> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:81
> > +    JSStringCache& stringCache = currentWorld(exec).m_stringCache;
> > +    JSStringCache::AddResult addResult = stringCache.add(stringImpl, nullptr);
> 
> I suggest using auto. I also suggest getting rid of the stringCache local variable.
> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:83
> > +        addResult.iterator->value = JSC::jsString(exec, String(stringImpl));
> 
> Seems a shame to do String(stringImpl) here instead of just using "s"

I'll get rid of the ref count churn here shortly. Will pretty up the names then too.
Comment 4 WebKit Commit Bot 2014-04-20 21:19:35 PDT
Comment on attachment 229780 [details]
Patch

Clearing flags on attachment: 229780

Committed r167577: <http://trac.webkit.org/changeset/167577>
Comment 5 WebKit Commit Bot 2014-04-20 21:19:38 PDT
All reviewed patches have been landed.  Closing bug.