Bug 108660 - Merged the global function cache into the source code cache
Summary: Merged the global function cache into the source code cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-01 10:49 PST by Geoffrey Garen
Modified: 2013-02-15 11:55 PST (History)
5 users (show)

See Also:


Attachments
Patch (15.95 KB, patch)
2013-02-01 11:30 PST, Geoffrey Garen
sam: review+
Details | Formatted Diff | Diff
benchmark results (1.40 KB, text/plain)
2013-02-01 11:31 PST, Geoffrey Garen
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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>