Bug 156317 - JSC should have a simple way of gathering IC statistics
Summary: JSC should have a simple way of gathering IC statistics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-06 16:39 PDT by Filip Pizlo
Modified: 2016-04-06 19:03 PDT (History)
8 users (show)

See Also:


Attachments
the patch (18.17 KB, patch)
2016-04-06 16:50 PDT, Filip Pizlo
keith_miller: review-
Details | Formatted Diff | Diff
the patch (27.46 KB, patch)
2016-04-06 18:10 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (27.92 KB, patch)
2016-04-06 18:43 PDT, Filip Pizlo
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-04-06 16:39:34 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-04-06 16:50:01 PDT
Created attachment 275831 [details]
the patch
Comment 2 Keith Miller 2016-04-06 16:57:15 PDT
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.
Comment 3 Filip Pizlo 2016-04-06 18:10:49 PDT
Created attachment 275841 [details]
the patch
Comment 4 WebKit Commit Bot 2016-04-06 18:12:28 PDT
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.
Comment 5 Filip Pizlo 2016-04-06 18:43:01 PDT
Created attachment 275844 [details]
the patch
Comment 6 WebKit Commit Bot 2016-04-06 18:44:40 PDT
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.
Comment 7 Benjamin Poulain 2016-04-06 18:46:36 PDT
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 :)
Comment 8 Filip Pizlo 2016-04-06 18:58:40 PDT
(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 :)
Comment 9 Filip Pizlo 2016-04-06 19:03:22 PDT
Landed in http://trac.webkit.org/changeset/199140