Bug 197905 - Make LOG_WITH_STREAM more efficient
Summary: Make LOG_WITH_STREAM more efficient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-14 23:15 PDT by Simon Fraser (smfr)
Modified: 2019-05-15 11:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.83 KB, patch)
2019-05-14 23:18 PDT, Simon Fraser (smfr)
achristensen: review+
Details | Formatted Diff | Diff
Patch (9.45 KB, patch)
2019-05-15 09:52 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>