Bug 110179 - Save space on keys in the CodeCache
Summary: Save space on keys in the CodeCache
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: 110242
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-18 22:41 PST by Geoffrey Garen
Modified: 2013-02-19 14:31 PST (History)
3 users (show)

See Also:


Attachments
Patch (4.96 KB, patch)
2013-02-18 22:59 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (4.34 KB, patch)
2013-02-19 09:32 PST, Geoffrey Garen
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2013-02-18 22:41:31 PST
Save space on keys in the CodeCache
Comment 1 Geoffrey Garen 2013-02-18 22:59:44 PST
Created attachment 188996 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-18 23:02:11 PST
Attachment 188996 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/CodeCache.cpp', u'Source/JavaScriptCore/runtime/CodeCache.h']" exit_code: 1
Source/JavaScriptCore/runtime/CodeCache.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/JavaScriptCore/runtime/CodeCache.h:61:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andreas Kling 2013-02-18 23:59:30 PST
Comment on attachment 188996 [details]
Patch

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

> Source/JavaScriptCore/runtime/CodeCache.h:86
> +    size_t length() const { return string().length(); }

Wouldn't it be cheaper to call m_sourceCode.length() here?
Comment 4 Geoffrey Garen 2013-02-19 08:35:48 PST
> > Source/JavaScriptCore/runtime/CodeCache.h:86
> > +    size_t length() const { return string().length(); }
> 
> Wouldn't it be cheaper to call m_sourceCode.length() here?

It would! Will fix.
Comment 5 Oliver Hunt 2013-02-19 09:08:23 PST
Comment on attachment 188996 [details]
Patch

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

> Source/JavaScriptCore/runtime/CodeCache.h:58
> -    enum CodeType { EvalType, ProgramType, FunctionCallType, FunctionConstructType };
> +    enum CodeType { EvalType, ProgramType, FunctionType };

Where is FunctionType used? I'm not sure where the CodeCache can directly interact with Functions now, and a brief looksy doesn't seem to imply that it does.  The FunctionType enum should be removed as well.
Comment 6 Geoffrey Garen 2013-02-19 09:32:28 PST
Created attachment 189104 [details]
Patch
Comment 7 Geoffrey Garen 2013-02-19 09:33:34 PST
> Where is FunctionType used?

FunctionType is used by 'new Function', which is still cached. (See getFunctionExecutableFromGlobalCode.)
Comment 8 Geoffrey Garen 2013-02-19 09:34:22 PST
Incorporated Andreas's fix, which in turn showed some more opportunities for simplifying based on m_sourceCode.
Comment 9 Oliver Hunt 2013-02-19 09:36:28 PST
Comment on attachment 189104 [details]
Patch

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

> Source/JavaScriptCore/runtime/CodeCache.h:94
> -            && m_sourceString == other.m_sourceString;
> +            && string() == other.string();

You could add a length check here as well, to further minimize the chance of having to incorrectly reify the string
Comment 10 Geoffrey Garen 2013-02-19 09:39:16 PST
> You could add a length check here as well, to further minimize the chance of having to incorrectly reify the string

Nice! I didn't do that before because length() was expensive; but now it's inexpensive, so I will.
Comment 11 Geoffrey Garen 2013-02-19 09:49:48 PST
Committed r143348: <http://trac.webkit.org/changeset/143348>
Comment 12 WebKit Review Bot 2013-02-19 11:32:53 PST
Re-opened since this is blocked by bug 110242
Comment 13 Geoffrey Garen 2013-02-19 14:31:16 PST
Re-landed in <http://trac.webkit.org/changeset/143384>.