Bug 206332 – Use dataLogIf more regularly

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

[Robin Morisset]

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.

[Robin Morisset]

Created attachment 387883 [details]
Patch

[Keith Miller]

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?

[Robin Morisset]

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?

[Robin Morisset]

Created attachment 387962 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387962 [details]
Patch

Clearing flags on attachment: 387962

Committed r254714: <https://trac.webkit.org/changeset/254714>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.