Bug 143506 – Lazily initialize LogToSystemConsole flag to reduce memory usage

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

When USE(CF) is defined, JSGlobalObjectConsoleClient::initializeLogToSystemConsole() calls into CoreFoundation preferences code to get application defaults.  In tests that can increase memory usage over 300K.  We should lazily call initializeLogToSystemConsole() to avoid this memory penalty.

[Michael Saboff]

rdar://problem/20443393

[Michael Saboff]

Created attachment 250321 [details]
Patch

Note: style failure is in existing code that got moved.

[WebKit Commit Bot]

Attachment 250321 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:50:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 250321 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:50
> +        std::call_once(initializeLogging, []{

Need a space between '[]' and '{'?

> Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:68
> +    // If setLogToSystemConsole() was called, no need to query the default value.
> +    if (sSetLogToSystemConsole)
> +        return;
> +

I think you can change this to an assert.  No one else should be calling this function anyway.  The original code was expecting to be only executed once only.

You can also make this function private in the header file.

[Michael Saboff]

(In reply to comment #4)
> Comment on attachment 250321 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250321&action=review
> 
> r=me

Thanks for the review.
 
> > Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:50
> > +        std::call_once(initializeLogging, []{
> 
> Need a space between '[]' and '{'?

The style bot wants the space, but I kept it with the original formatting as I think that adjacent makes more sense.

> > Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:68
> > +    // If setLogToSystemConsole() was called, no need to query the default value.
> > +    if (sSetLogToSystemConsole)
> > +        return;
> > +
> 
> I think you can change this to an assert.  No one else should be calling
> this function anyway.  The original code was expecting to be only executed
> once only.

Actually that if is needed.  In the prior code, this run-once initializer always ran before the setter and getter could be called.  Now, the setter could be called before this run-once function and we don't want this run-once initializers to possible overwrite the value of sLogToSystemConsole.  This "if" protects that case.

> You can also make this function private in the header file.

I'll do that.

[Michael Saboff]

Committed r182535: <http://trac.webkit.org/changeset/182535>

[Geoffrey Garen]

I don't think this fix is sufficient, since the memory use will still happen after the first call to console.log.

We've seen -- both in the web space and the embedded space -- that client code likes to leave in all its debugging console.log calls even in production. So, we need to make this memory savings stick even after the first call to console.log.

This should be achievable by just arranging not to do any of this work when the inspector is disabled. JoePeck recently made a change like this.

[Michael Saboff]

(In reply to comment #7)
> I don't think this fix is sufficient, since the memory use will still happen
> after the first call to console.log.
> 
> We've seen -- both in the web space and the embedded space -- that client
> code likes to leave in all its debugging console.log calls even in
> production. So, we need to make this memory savings stick even after the
> first call to console.log.
> 
> This should be achievable by just arranging not to do any of this work when
> the inspector is disabled. JoePeck recently made a change like this.

If we disable the inspector at compile time, we don't hit this path.

If the inspector is enabled at compile time, we need to store the console output for a possible subsequent attach by a remote inspector.

The memory usage is due to calling into CFPreferencesGetAppBooleanValue().

[Geoffrey Garen]

> If the inspector is enabled at compile time, we need to store the console
> output for a possible subsequent attach by a remote inspector.

No. We can detect that the inspector is not enabled.

> The memory usage is due to calling into CFPreferencesGetAppBooleanValue().

Got it.