ResourceBundle calling UTF8ToUTF16 is on the browser startup critical path, and not needed (and seems kinda slow?) |
|||||||
Issue descriptionChrome Version : 64.0.3251.0 I'm poking around the call stacks in my performance regression (Issue 780760), and it seems weird there's a call to base::UTF8ToUTF16() taking up 7.5ms of browser startup. https://uma.googleplex.com/p/chrome/callstacks?sid=59bf7d6acf16768a3b13d47d3f4f9926 The code is under ProfileAttributesStorage::IsDefaultProfileName(..) when it calls l10n_util::GetStringFUTF8(IDS_NEW_NUMBERED_PROFILE_NAME, ..) -- note it wants a UTF8 string, so why is UTF8ToUTF16 being called at all? Well, l10n_util is just std::string GetStringUTF8(int message_id) { return base::UTF16ToUTF8(GetStringUTF16(message_id)); } ResourceBundle only returns UTF16 strings, but this string is actually stored as UTF8. ResourceBundle needs to convert it: // Data pack encodes strings as either UTF8 or UTF16. base::string16 msg; if (encoding == ResourceHandle::UTF16) { msg = base::string16(reinterpret_cast<const base::char16*>(data.data()), data.length() / 2); } else if (encoding == ResourceHandle::UTF8) { msg = base::UTF8ToUTF16(data); } return msg; Seems there is an easy win in startup performance (and performance in general) if we just add a version of ResourceBundle::GetLocalizedString that returns UTF8, and have l10n_util::GetStringUTF8() call it instead. Alternatively, data pack could *always* encode as utf16, but that probably wastes memory/disk/package space.
,
Nov 7 2017
Sounds like the extra string conversions are not necessary, but I believe the 7.5ms execution time here is actually the cost of loading the memory-mapped string resource from disk. All the runtime is in base::IsStringASCII(), which is the first place that string contents are actually touched by the code. From working with this data I've found that a significant chunk of the startup time is doing disk I/O, much of it implicitly via memory-mapped resources.
,
Nov 8 2017
Has anyone tried triggering the page faults on a background thread? Maybe we can store a LocalPref that just lists the first ~20 resource_ids that ResourceBundle loaded last browser startup, and request them on a worker thread as the first step of PreMainMessageLoopRun. Or pass them as an argument to AddDataPackFromPath at ChromeBrowserMainParts::PreCreateThreadsImpl:AddDataPack These page faults should be able to be interleaved with the typesetting and FontServer IPC that needs to happen during startup.
,
Jan 5 2018
Somehow missed the last comment on this bug. A little while back Alexei reordered the resources to be in the same order as loaded at startup[1], but I'm not aware of any coordinated effort to load things on a background thread. I believe that an overarching effort to be even smarter about disk I/O at startup could generate additional significant reductions in startup time. [1] https://chromium.googlesource.com/chromium/src/tools/grit/+/0e4577338a531a76058a3471e6a6959495b420d2
,
Jan 5 2018
I don't think anyone has tried that. The resource ordering stuff would make the logic for this kind of background-thread pre-read very simple - since all the startup resources would be in-order at the start of the file. So it would be a matter of starting a thread that preread the start of the resource bundle file. +fdoray +gab in case they may be interested in this.
,
Jan 10 2018
,
Jan 18 2018
,
Jan 18
(5 days ago)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 18
(5 days ago)
I think WontFix is correct here, since the cause of slow down is not actually UTF8ToUTF16() but resource loading causing page faults for the mmaped file, for which we now have resource ordering set up to reduce page loads. Marking as WontFix. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by tapted@chromium.org
, Nov 2 2017Status: Available (was: Started)