143506 2015-04-07 17:58:29 -0700 Lazily initialize LogToSystemConsole flag to reduce memory usage 2015-04-08 11:28:10 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 msaboff msaboff commit-queue ggaren oldest_to_newest 1083503 0 msaboff 2015-04-07 17:58:29 -0700 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. 1083504 1 msaboff 2015-04-07 17:59:04 -0700 rdar://problem/20443393 1083505 2 250321 msaboff 2015-04-07 18:05:46 -0700 Created attachment 250321 Patch Note: style failure is in existing code that got moved. 1083506 3 commit-queue 2015-04-07 18:08:16 -0700 Attachment 250321 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. 1083513 4 250321 mark.lam 2015-04-07 18:49:26 -0700 Comment on attachment 250321 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. 1083559 5 msaboff 2015-04-07 23:19:04 -0700 (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. 1083601 6 msaboff 2015-04-08 07:04:18 -0700 Committed r182535: <http://trac.webkit.org/changeset/182535> 1083660 7 ggaren 2015-04-08 11:04:44 -0700 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. 1083664 8 msaboff 2015-04-08 11:12:29 -0700 (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(). 1083671 9 ggaren 2015-04-08 11:28:10 -0700 > 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. 250321 2015-04-07 18:05:46 -0700 2015-04-07 18:49:44 -0700 Patch 143506.patch text/plain 2918 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTgyNTExKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDE1LTA0LTA3ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIExhemlseSBpbml0aWFsaXplIExvZ1RvU3lzdGVtQ29uc29sZSBmbGFnIHRvIHJlZHVjZSBt ZW1vcnkgdXNhZ2UKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE0MzUwNgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIE9ubHkgY2FsbCBpbnRvIENGIHByZWZlcmVuY2VzIGNvZGUgd2hlbiB3ZSBuZWVkIHRvIGlu IG9yZGVyIHRvIHJlZHVjZSBtZW1vcnkgdXNhZ2UuCisKKyAgICAgICAgKiBpbnNwZWN0b3IvSlNH bG9iYWxPYmplY3RDb25zb2xlQ2xpZW50LmNwcDoKKyAgICAgICAgKEluc3BlY3Rvcjo6SlNHbG9i YWxPYmplY3RDb25zb2xlQ2xpZW50Ojpsb2dUb1N5c3RlbUNvbnNvbGUpOgorICAgICAgICAoSW5z cGVjdG9yOjpKU0dsb2JhbE9iamVjdENvbnNvbGVDbGllbnQ6OnNldExvZ1RvU3lzdGVtQ29uc29s ZSk6CisgICAgICAgIChJbnNwZWN0b3I6OkpTR2xvYmFsT2JqZWN0Q29uc29sZUNsaWVudDo6aW5p dGlhbGl6ZUxvZ1RvU3lzdGVtQ29uc29sZSk6CisgICAgICAgIChJbnNwZWN0b3I6OkpTR2xvYmFs T2JqZWN0Q29uc29sZUNsaWVudDo6SlNHbG9iYWxPYmplY3RDb25zb2xlQ2xpZW50KToKKwogMjAx NS0wNC0wNyAgQmVuamFtaW4gUG91bGFpbiAgPGJlbmphbWluQHdlYmtpdC5vcmc+CiAKICAgICAg ICAgR2V0IHRoZSBmZWF0dXJlcy5qc29uIGZpbGVzIHJlYWR5IGZvciBvcGVuIGNvbnRyaWJ1dGlv bnMKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnNwZWN0b3IvSlNHbG9iYWxPYmplY3RD b25zb2xlQ2xpZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaW5z cGVjdG9yL0pTR2xvYmFsT2JqZWN0Q29uc29sZUNsaWVudC5jcHAJKHJldmlzaW9uIDE4MjUwNCkK KysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnNwZWN0b3IvSlNHbG9iYWxPYmplY3RDb25zb2xl Q2xpZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDEsMTkgKzQxLDMxIEBAIHVzaW5nIG5hbWVz cGFjZSBKU0M7CiBuYW1lc3BhY2UgSW5zcGVjdG9yIHsKIAogc3RhdGljIGJvb2wgc0xvZ1RvU3lz dGVtQ29uc29sZSA9IGZhbHNlOworc3RhdGljIGJvb2wgc1NldExvZ1RvU3lzdGVtQ29uc29sZSA9 IGZhbHNlOwogCiBib29sIEpTR2xvYmFsT2JqZWN0Q29uc29sZUNsaWVudDo6bG9nVG9TeXN0ZW1D b25zb2xlKCkKIHsKKyAgICBpZiAoIXNTZXRMb2dUb1N5c3RlbUNvbnNvbGUpIHsKKyAgICAgICAg c3RhdGljIHN0ZDo6b25jZV9mbGFnIGluaXRpYWxpemVMb2dnaW5nOworICAgICAgICBzdGQ6OmNh bGxfb25jZShpbml0aWFsaXplTG9nZ2luZywgW117CisgICAgICAgICAgICBKU0dsb2JhbE9iamVj dENvbnNvbGVDbGllbnQ6OmluaXRpYWxpemVMb2dUb1N5c3RlbUNvbnNvbGUoKTsKKyAgICAgICAg fSk7CisgICAgfQogICAgIHJldHVybiBzTG9nVG9TeXN0ZW1Db25zb2xlOwogfQogCiB2b2lkIEpT R2xvYmFsT2JqZWN0Q29uc29sZUNsaWVudDo6c2V0TG9nVG9TeXN0ZW1Db25zb2xlKGJvb2wgc2hv dWxkTG9nKQogeworICAgIHNTZXRMb2dUb1N5c3RlbUNvbnNvbGUgPSB0cnVlOwogICAgIHNMb2dU b1N5c3RlbUNvbnNvbGUgPSBzaG91bGRMb2c7CiB9CiAKIHZvaWQgSlNHbG9iYWxPYmplY3RDb25z b2xlQ2xpZW50Ojppbml0aWFsaXplTG9nVG9TeXN0ZW1Db25zb2xlKCkKIHsKKyAgICAvLyBJZiBz ZXRMb2dUb1N5c3RlbUNvbnNvbGUoKSB3YXMgY2FsbGVkLCBubyBuZWVkIHRvIHF1ZXJ5IHRoZSBk ZWZhdWx0IHZhbHVlLgorICAgIGlmIChzU2V0TG9nVG9TeXN0ZW1Db25zb2xlKQorICAgICAgICBy ZXR1cm47CisKICNpZiAhTE9HX0RJU0FCTEVECiAgICAgc0xvZ1RvU3lzdGVtQ29uc29sZSA9IHRy dWU7CiAjZWxpZiBVU0UoQ0YpCkBAIC02MiwxNiArNzQsMTMgQEAgdm9pZCBKU0dsb2JhbE9iamVj dENvbnNvbGVDbGllbnQ6OmluaXRpYQogICAgIGlmIChrZXlFeGlzdHNBbmRIYXNWYWxpZEZvcm1h dCkKICAgICAgICAgc0xvZ1RvU3lzdGVtQ29uc29sZSA9IHByZWZlcmVuY2U7CiAjZW5kaWYKKyAg ICBzU2V0TG9nVG9TeXN0ZW1Db25zb2xlID0gdHJ1ZTsKIH0KIAogSlNHbG9iYWxPYmplY3RDb25z b2xlQ2xpZW50OjpKU0dsb2JhbE9iamVjdENvbnNvbGVDbGllbnQoSW5zcGVjdG9yQ29uc29sZUFn ZW50KiBjb25zb2xlQWdlbnQpCiAgICAgOiBDb25zb2xlQ2xpZW50KCkKICAgICAsIG1fY29uc29s ZUFnZW50KGNvbnNvbGVBZ2VudCkKIHsKLSAgICBzdGF0aWMgc3RkOjpvbmNlX2ZsYWcgaW5pdGlh bGl6ZUxvZ2dpbmc7Ci0gICAgc3RkOjpjYWxsX29uY2UoaW5pdGlhbGl6ZUxvZ2dpbmcsIFtdewot ICAgICAgICBKU0dsb2JhbE9iamVjdENvbnNvbGVDbGllbnQ6OmluaXRpYWxpemVMb2dUb1N5c3Rl bUNvbnNvbGUoKTsKLSAgICB9KTsKIH0KIAogdm9pZCBKU0dsb2JhbE9iamVjdENvbnNvbGVDbGll bnQ6Om1lc3NhZ2VXaXRoVHlwZUFuZExldmVsKE1lc3NhZ2VUeXBlIHR5cGUsIE1lc3NhZ2VMZXZl bCBsZXZlbCwgSlNDOjpFeGVjU3RhdGUqIGV4ZWMsIFJlZlB0cjxTY3JpcHRBcmd1bWVudHM+JiYg YXJndW1lbnRzKQo=