Bug 143506 - Lazily initialize LogToSystemConsole flag to reduce memory usage
Summary: Lazily initialize LogToSystemConsole flag to reduce memory usage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-07 17:58 PDT by Michael Saboff
Modified: 2015-04-08 11:28 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.85 KB, patch)
2015-04-07 18:05 PDT, Michael Saboff
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2015-04-07 17:58:29 PDT
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.
Comment 1 Michael Saboff 2015-04-07 17:59:04 PDT
rdar://problem/20443393
Comment 2 Michael Saboff 2015-04-07 18:05:46 PDT
Created attachment 250321 [details]
Patch

Note: style failure is in existing code that got moved.
Comment 3 WebKit Commit Bot 2015-04-07 18:08:16 PDT
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.
Comment 4 Mark Lam 2015-04-07 18:49:26 PDT
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.
Comment 5 Michael Saboff 2015-04-07 23:19:04 PDT
(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.
Comment 6 Michael Saboff 2015-04-08 07:04:18 PDT
Committed r182535: <http://trac.webkit.org/changeset/182535>
Comment 7 Geoffrey Garen 2015-04-08 11:04:44 PDT
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.
Comment 8 Michael Saboff 2015-04-08 11:12:29 PDT
(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().
Comment 9 Geoffrey Garen 2015-04-08 11:28:10 PDT
> 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.