Bug 156317 – JSC should have a simple way of gathering IC statistics

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 275831 [details]
the patch

[Keith Miller]

Comment on attachment 275831 [details]
the patch

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

I think you forgot to include ICStats.h also.

> Source/JavaScriptCore/jit/JITOperations.cpp:187
> +    if (ident == "supersedes") {
> +        dataLog(*exec->codeBlock(), " ", exec->codeOrigin(), ": ");
> +        switch (stubInfo->cacheType) {
> +        case CacheType::Unset:
> +            dataLog("Unset");
> +            break;
> +        case CacheType::GetByIdSelf:
> +            dataLog("GetByIdSelf");
> +            break;
> +        case CacheType::Stub:
> +            dataLog("Stub(", pointerDump(stubInfo->u.stub), ")");
> +            break;
> +        default:
> +            RELEASE_ASSERT_NOT_REACHED();
> +            break;
> +        }
> +        dataLog(" for ", baseValue, "\n");
> +    }

This should be deleted before landing.

[Filip Pizlo]

Created attachment 275841 [details]
the patch

[WebKit Commit Bot]

Attachment 275841 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/ICStats.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/ICStats.h:167:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/ICStats.cpp:63:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/ICStats.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 275844 [details]
the patch

[WebKit Commit Bot]

Attachment 275844 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/ICStats.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/ICStats.h:167:  The parameter name "event" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/ICStats.cpp:63:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/ICStats.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 275844 [details]
the patch

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

> Source/JavaScriptCore/jit/ICStats.h:115
> +        return m_kind + WTF::PtrHash<const ClassInfo*>::hash(m_classInfo) + StringHash::hash(m_propertyName.string());

+ tends to suck hard for combining hashes.
PairHash would probably be better here.

> Source/JavaScriptCore/jit/ICStats.h:125
> +    void log() const;

Odd API IMHO. 

I would not mind the explicit ICStats::instance().add().

> Source/JavaScriptCore/jit/ICStats.h:182
> +#define LOG_IC(arguments) do {                  \

No way to use a template to make this into a real function instead of a macro?

> Source/WTF/ChangeLog:10
> +        std::chrono. I now believe that std::chrono is just a bad decision, and I always want to
> +        use doubles instead. This makes it easier to do the right thing and use doubles.

ahahahah. I dislike chrono too :)

[Filip Pizlo]

(In reply to comment #7)
> Comment on attachment 275844 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275841&action=review
> 
> > Source/JavaScriptCore/jit/ICStats.h:115
> > +        return m_kind + WTF::PtrHash<const ClassInfo*>::hash(m_classInfo) + StringHash::hash(m_propertyName.string());
> 
> + tends to suck hard for combining hashes.
> PairHash would probably be better here.
> 
> > Source/JavaScriptCore/jit/ICStats.h:125
> > +    void log() const;
> 
> Odd API IMHO. 
> 
> I would not mind the explicit ICStats::instance().add().

Yeah...  Since there is a call to this in 6 places or so, I was trying to conserve code size by having just one call.

> 
> > Source/JavaScriptCore/jit/ICStats.h:182
> > +#define LOG_IC(arguments) do {                  \
> 
> No way to use a template to make this into a real function instead of a
> macro?

You could but it would be weird.  The point is to avoid evaluating the arguments if the flag is not set, since those arguments require work to evaluate (particularly the call to classInfoOrNull(), but not just that).

I think that if we didn't like the macro, we could just have JITOperations.cpp explicitly test Options::useICStats().  I just wanted the brevity so I went with the macro.

> 
> > Source/WTF/ChangeLog:10
> > +        std::chrono. I now believe that std::chrono is just a bad decision, and I always want to
> > +        use doubles instead. This makes it easier to do the right thing and use doubles.
> 
> ahahahah. I dislike chrono too :)

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/199140