There is lots of code that reads if(Options::foobar()) dataLogLn("...") There are a couple of benefits to replacing those by dataLogLnIf(Options::foobar(), "..."): - Readability, by reducing the number of lines taken by logging - Less lines appearing as not-taken in test coverage wrongly (wrongly because we probably don't care for the coverage of logging code) - possibly a tiny perf benefit since dataLogIf correctly uses UNLIKELY. This patch is a fairly trivial refactoring where I looked for that pattern and replaced it everywhere it appeared in JSC.
Created attachment 387883 [details] Patch
Comment on attachment 387883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387883&action=review r=me with nits. > Source/JavaScriptCore/ftl/FTLOSREntry.cpp:64 > + dataLogLnIf(Options::verboseOSR()," OSR failed because we don't have an entrypoint for ", bytecodeIndex, "; ours is for ", entryCode->bytecodeIndex()); Can you add a space here? > Source/JavaScriptCore/ftl/FTLOSREntry.cpp:71 > + dataLogLnIf(Options::verboseOSR()," Values at entry: ", values); ditto. > Source/JavaScriptCore/ftl/FTLOSREntry.cpp:100 > + dataLogLnIf(Options::verboseOSR()," OSR failed because stack growth failed."); ditto. > Source/JavaScriptCore/ftl/FTLOSREntry.cpp:107 > + dataLogLnIf(Options::verboseOSR()," Entry will succeed, going to address ", RawPointer(result)); Ditto. > Source/WTF/ChangeLog:9 > + (WTF::dataLog): Marked NEVER_INLINE Why never inline this?
Thanks for the review. (In reply to Keith Miller from comment #2) > Comment on attachment 387883 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387883&action=review > > r=me with nits. > > > Source/JavaScriptCore/ftl/FTLOSREntry.cpp:64 > > + dataLogLnIf(Options::verboseOSR()," OSR failed because we don't have an entrypoint for ", bytecodeIndex, "; ours is for ", entryCode->bytecodeIndex()); > > Can you add a space here? Fixed, as well as the other ones. > > Source/WTF/ChangeLog:9 > > + (WTF::dataLog): Marked NEVER_INLINE > > Why never inline this? Because if logging is on the critical path for performance we have much bigger problems to be concerned about, and I don't want to have the compiler waste time trying to optimize the way we call logging code?
Created attachment 387962 [details] Patch
Comment on attachment 387962 [details] Patch Clearing flags on attachment: 387962 Committed r254714: <https://trac.webkit.org/changeset/254714>
All reviewed patches have been landed. Closing bug.