Bug 90228 – Do not do any logging initialization when logging is disabled

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Benjamin Poulain]

That takes time on start up.

<rdar://problem/11746350>

[Benjamin Poulain]

Created attachment 150044 [details]
Patch

[Early Warning System Bot]

Comment on attachment 150044 [details]
Patch

Attachment 150044 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13108240

[Gustavo Noronha (kov)]

Comment on attachment 150044 [details]
Patch

Attachment 150044 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13119163

[Early Warning System Bot]

Comment on attachment 150044 [details]
Patch

Attachment 150044 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13114181

[Benjamin Poulain]

Created attachment 150055 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 150055 [details]
Patch

Attachment 150055 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13121124

[Benjamin Poulain]

Created attachment 150059 [details]
Patch

[Benjamin Poulain]

Looks like GTK has a strange use of WebCore logging :(

[Simon Fraser (smfr)]

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.

[Joseph Pecoraro]

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"

[Benjamin Poulain]

Created attachment 150263 [details]
Patch

[Benjamin Poulain]

Comment on attachment 150263 [details]
Patch

I'll look into using Assertions.h directly for WebCore.exp.in.

[Joseph Pecoraro]

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 =)

[Benjamin Poulain]

Created attachment 150297 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150297 [details]
Patch

Looks find on EFL port case.

[Benjamin Poulain]

Committed r121707: <http://trac.webkit.org/changeset/121707>