Bug 110179 – Save space on keys in the CodeCache

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Save space on keys in the CodeCache

[Geoffrey Garen]

Created attachment 188996 [details]
Patch

[WebKit Review Bot]

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.

[Andreas Kling]

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?

[Geoffrey Garen]

> > 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.

[Oliver Hunt]

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.

[Geoffrey Garen]

Created attachment 189104 [details]
Patch

[Geoffrey Garen]

> Where is FunctionType used?

FunctionType is used by 'new Function', which is still cached. (See getFunctionExecutableFromGlobalCode.)

[Geoffrey Garen]

Incorporated Andreas's fix, which in turn showed some more opportunities for simplifying based on m_sourceCode.

[Oliver Hunt]

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

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

Committed r143348: <http://trac.webkit.org/changeset/143348>

[WebKit Review Bot]

Re-opened since this is blocked by bug 110242

[Geoffrey Garen]

Re-landed in <http://trac.webkit.org/changeset/143384>.