Bug 197905

Summary: Make LOG_WITH_STREAM more efficient
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, sam, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
achristensen: review+
Patch none

Description Simon Fraser (smfr) 2019-05-14 23:15:38 PDT
Make LOG_WITH_STREAM more efficient
Comment 1 Simon Fraser (smfr) 2019-05-14 23:18:44 PDT
Created attachment 369929 [details]
Patch
Comment 2 Alex Christensen 2019-05-15 07:48:13 PDT
Comment on attachment 369929 [details]
Patch

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

> Source/WTF/wtf/text/TextStream.h:109
> +    struct repeat {
> +        repeat(char inCharacter, unsigned inWidth)
> +            : width(inWidth)
> +            , character(inCharacter)
> +        { }

I think this should be named Repeat.
I think initializer lists could be used instead of this constructor.

> Source/WebCore/PAL/pal/LogMacros.h:35
> +        if (JOIN_LOG_CHANNEL_WITH_PREFIX(LOG_CHANNEL_PREFIX, channel).state == WTFLogChannelState::On) { \

This seems like an obvious check we should've done before.
Comment 3 Simon Fraser (smfr) 2019-05-15 08:41:54 PDT
(In reply to Alex Christensen from comment #2)
> Comment on attachment 369929 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369929&action=review
> 
> > Source/WTF/wtf/text/TextStream.h:109
> > +    struct repeat {
> > +        repeat(char inCharacter, unsigned inWidth)
> > +            : width(inWidth)
> > +            , character(inCharacter)
> > +        { }
> 
> I think this should be named Repeat.
> I think initializer lists could be used instead of this constructor.

So the call site would look like:
stream << Repeat({' ', 10}) << "foopy"
?
Comment 4 Alex Christensen 2019-05-15 09:03:05 PDT
(In reply to Simon Fraser (smfr) from comment #3)
stream << Repeat {' ', 10} << "foopy"
Comment 5 Simon Fraser (smfr) 2019-05-15 09:24:08 PDT
Sadly the compiler doesn't like initializers inside a macro invocation:
/rendering/RenderLayerCompositor.cpp:1176:69: error: too many arguments provided to function-like macro invocation
    LOG_WITH_STREAM(Compositing, stream << TextStream::Repeat {' ', compositingState.depth * 2} << &layer << " computeCompositingRequirements - willBeComposited " << willBeComposited << " (backing provider candidate " << backingSharingState.backingProviderCandidate() << ")");
Comment 6 Simon Fraser (smfr) 2019-05-15 09:52:11 PDT
Created attachment 369961 [details]
Patch
Comment 7 Simon Fraser (smfr) 2019-05-15 11:54:28 PDT
https://trac.webkit.org/r245336
Comment 8 Radar WebKit Bug Importer 2019-05-15 11:58:02 PDT
<rdar://problem/50820136>