Bug 90228

Summary: Do not do any logging initialization when logging is disabled
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: WebKit Misc.Assignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, danw, ddkilzer, eric.carlson, eric, feature-media-reviews, gns, japhet, jochen, joepeck, menard, mifenton, mrobinson, pnormand, rakuco, webkit.review.bot, xan.lopez, zoltan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review+

Description Benjamin Poulain 2012-06-28 17:00:40 PDT
That takes time on start up.

<rdar://problem/11746350>
Comment 1 Benjamin Poulain 2012-06-28 17:11:15 PDT
Created attachment 150044 [details]
Patch
Comment 2 Early Warning System Bot 2012-06-28 17:39:07 PDT
Comment on attachment 150044 [details]
Patch

Attachment 150044 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13108240
Comment 3 Gustavo Noronha (kov) 2012-06-28 17:59:02 PDT
Comment on attachment 150044 [details]
Patch

Attachment 150044 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13119163
Comment 4 Early Warning System Bot 2012-06-28 18:08:52 PDT
Comment on attachment 150044 [details]
Patch

Attachment 150044 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13114181
Comment 5 Benjamin Poulain 2012-06-28 18:09:23 PDT
Created attachment 150055 [details]
Patch
Comment 6 Gustavo Noronha (kov) 2012-06-28 18:20:22 PDT
Comment on attachment 150055 [details]
Patch

Attachment 150055 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13121124
Comment 7 Benjamin Poulain 2012-06-28 18:25:40 PDT
Created attachment 150059 [details]
Patch
Comment 8 Benjamin Poulain 2012-06-28 18:26:05 PDT
Looks like GTK has a strange use of WebCore logging :(
Comment 9 Simon Fraser (smfr) 2012-06-29 13:00:01 PDT
Comment on attachment 150059 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Initializating of the logging channels was taking time on startup. When loggin is disabled

"loggin"

> Source/WebCore/ChangeLog:9
> +        (and the LOG macro do nothing), we should aslo disable logging channels and initialization.

do nothing -> does nothing. aslo.

> Source/WebCore/ChangeLog:12
> +        The biggest part of the patch is the change from !LOG_DISABLED to using ENABLE(LOGGING).
> +        This is done for consistency with the other ENABLE features used in WebKit.

But I don't think logging is like other ENABLE features. ENABLE is used for  web- or user- facing features, and doesn't normally change between release and debug builds. LOGGING is more like ASSERTS, in that it's disabled in release.
Comment 10 Joseph Pecoraro 2012-06-29 13:02:32 PDT
Comment on attachment 150059 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Initializating of the logging channels was taking time on startup. When loggin is disabled
> +        (and the LOG macro do nothing), we should aslo disable logging channels and initialization.

Nit (typo): "Initializating" => "Initializing"
Nit (typo): "loggin" => "logging"
Nit (typo): "macro do nothing" => "macro does nothing"
Nit (typo): "aslo" => "also"
Comment 11 Benjamin Poulain 2012-06-29 15:25:52 PDT
Created attachment 150263 [details]
Patch
Comment 12 Benjamin Poulain 2012-06-29 16:51:22 PDT
Comment on attachment 150263 [details]
Patch

I'll look into using Assertions.h directly for WebCore.exp.in.
Comment 13 Joseph Pecoraro 2012-06-29 18:30:31 PDT
Comment on attachment 150263 [details]
Patch

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

> Source/WebCore/WebCore.exp.in:631
> +#if !LOG_DISABLED
>  __ZN7WebCore36initializeLoggingChannelsIfNecessaryEv
> +#endif // !LOG_DISABLED

The convention seems to be adding a new block near the bottom of the file.

    #if !defined(LOG_DISABLED)
    ....
    #endif

I assume other build systems (like Windows?) might need a similar export fix.

This patch is much easier to follow =)
Comment 14 Benjamin Poulain 2012-06-29 20:25:19 PDT
Created attachment 150297 [details]
Patch
Comment 15 Gyuyoung Kim 2012-07-01 23:04:43 PDT
Comment on attachment 150297 [details]
Patch

Looks find on EFL port case.
Comment 16 Benjamin Poulain 2012-07-02 14:48:19 PDT
Committed r121707: <http://trac.webkit.org/changeset/121707>