Bug 156317

Summary: JSC should have a simple way of gathering IC statistics
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
keith_miller: review-
the patch
none
the patch benjamin: review+

Filip Pizlo
Reported 2016-04-06 16:39:34 PDT
Patch forthcoming.
Attachments
the patch (18.17 KB, patch)
2016-04-06 16:50 PDT, Filip Pizlo
keith_miller: review-
the patch (27.46 KB, patch)
2016-04-06 18:10 PDT, Filip Pizlo
no flags
the patch (27.92 KB, patch)
2016-04-06 18:43 PDT, Filip Pizlo
benjamin: review+
Filip Pizlo
Comment 1 2016-04-06 16:50:01 PDT
Created attachment 275831 [details] the patch
Keith Miller
Comment 2 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.
Filip Pizlo
Comment 3 2016-04-06 18:10:49 PDT
Created attachment 275841 [details] the patch
WebKit Commit Bot
Comment 4 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.
Filip Pizlo
Comment 5 2016-04-06 18:43:01 PDT
Created attachment 275844 [details] the patch
WebKit Commit Bot
Comment 6 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.
Benjamin Poulain
Comment 7 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 :)
Filip Pizlo
Comment 8 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 :)
Filip Pizlo
Comment 9 2016-04-06 19:03:22 PDT
Note You need to log in before you can comment on or make changes to this bug.