Bug 108660

Summary: Merged the global function cache into the source code cache
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, kling, oliver, yongjun.zhang
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
sam: review+
benchmark results none

Description Geoffrey Garen 2013-02-01 10:49:13 PST
Merged the global function cache into the source code cache
Comment 1 Geoffrey Garen 2013-02-01 11:30:06 PST
Created attachment 186085 [details]
Patch
Comment 2 Geoffrey Garen 2013-02-01 11:31:21 PST
Created attachment 186086 [details]
benchmark results
Comment 3 Oliver Hunt 2013-02-01 11:38:37 PST
Could you test on http://jsbench.cs.purdue.edu as well? and the plt?
Comment 4 Geoffrey Garen 2013-02-01 12:08:33 PST
> Could you test on http://jsbench.cs.purdue.edu as well?

I did. Same result.
Comment 5 Geoffrey Garen 2013-02-14 22:39:01 PST
> and the plt?

Had to bump the limit to 1280 to make the plt happy. Seems to confirm the need for an adaptive size.
Comment 6 Geoffrey Garen 2013-02-14 22:40:03 PST
> > and the plt?

(Merging the caches and keeping the net limit of 2048 is a 4% PLT speedup.)
Comment 7 Geoffrey Garen 2013-02-14 22:42:40 PST
Committed r142966: <http://trac.webkit.org/changeset/142966>
Comment 8 Darin Adler 2013-02-15 09:20:51 PST
Comment on attachment 186085 [details]
Patch

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

> Source/JavaScriptCore/runtime/CodeCache.h:102
> +        , m_name(WTF::HashTableDeletedValue)

I think this not helpful. Instead m_name should be allowed to initialize itself to the null string. m_sourceString is the field that holds the deleted state and m_name need not do anything special. In fact, if there was an easy way to do so, I would suggest leaving this in an uninitialized state.

> Source/JavaScriptCore/runtime/CodeCache.h:103
> +        , m_flags(0)

Why not omit this? Why generate the extra code to zero out the m_flags field?

> Source/JavaScriptCore/runtime/CodeCache.h:132
> +struct SourceCodeKeyHashTraits : WTF::SimpleClassHashTraits<SourceCodeKey> {

Surprised you have to use the WTF:: prefix here. Should we fix that in WTF by putting adding using lines to the HashTraits.h header?
Comment 9 Geoffrey Garen 2013-02-15 11:55:16 PST
> > Source/JavaScriptCore/runtime/CodeCache.h:102
> > +        , m_name(WTF::HashTableDeletedValue)
> 
> I think this not helpful. Instead m_name should be allowed to initialize itself to the null string. m_sourceString is the field that holds the deleted state and m_name need not do anything special. In fact, if there was an easy way to do so, I would suggest leaving this in an uninitialized state.
> 
> > Source/JavaScriptCore/runtime/CodeCache.h:103
> > +        , m_flags(0)
> 
> Why not omit this? Why generate the extra code to zero out the m_flags field?

Agreed on both counts. Will fix.

> > Source/JavaScriptCore/runtime/CodeCache.h:132
> > +struct SourceCodeKeyHashTraits : WTF::SimpleClassHashTraits<SourceCodeKey> {
> 
> Surprised you have to use the WTF:: prefix here. Should we fix that in WTF by putting adding using lines to the HashTraits.h header?

Sure.
Comment 10 Geoffrey Garen 2013-02-15 11:55:39 PST
Committed r143027: <http://trac.webkit.org/changeset/143027>