1377377 - Blocking on reading in string bundle to report error message to console slows down Facebook group page load

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mats Palmgren (inactive)]

Spawned off from bug 1375346 comment 3:
> Spending 95ms (3.9%) on waiting to read in error messages kind of sucks.
> Maybe we can preload string bundles that we expect to almost always be
> used some time shortly after startup (as a background task)?  Or can we
> make reporting errors to the console async?
> 
> ... Facebook script code ...
> a/j.prototype.getView
> h
> 0x1f6f0dba4d5
> mozilla::dom::GenericBindingMethod(JSContext *,unsigned int,JS::Value *)
> mozilla::dom::DocumentBinding::getElementById
> nsDocument::GetElementById(nsAString const &)
> nsDocument::ReportEmptyGetElementByIdArg()
> nsContentUtils::ReportToConsole(unsigned int,nsACString const &,nsIDocument
> const *,nsContentUtils::PropertiesFile,char const *,char16_t const *
> *,unsigned int,nsIURI *,nsString const &,unsigned int,unsigned int)
> nsContentUtils::GetLocalizedString(nsContentUtils::PropertiesFile,char const
> *,nsXPIDLString &)
> nsStringBundle::GetStringFromName(char16_t const *,char16_t * *)
> nsStringBundle::LoadProperties()
> nsPersistentProperties::Load(nsIInputStream *)
> UTF8InputStream::ReadSegments(nsresult (*)(nsIUnicharInputStream *,void
> *,char16_t const *,unsigned int,unsigned int,unsigned int *),void *,unsigned
> int,unsigned int *)
> UTF8InputStream::Fill(nsresult *)
> NS_FillArray(FallibleTArray<char> &,nsIInputStream *,unsigned int,unsigned
> int *)
> nsJARInputStream::Read(char *,unsigned int,unsigned int *)
> 0x7ffc6e88c387 (top of stack)

Comment 1

[Andrew Overholt [:overholt]]

Boris, do you know what should do here? Some sort of caching?

Comment 2

[Boris Zbarsky [:bzbarsky]]

We already do caching.  The above stack will only happen the first time we try to do something with the "dom.properties" file.  After that the stringbundle will in fact cache all those strings and we won't have to hit the disk again.

I think comment 0 describes the options: either we should load the stringbundles in more eagerly, at idle time, or we should make the whole thing async.  The latter has its own problems: it can confuse web developers if things in the console are mis-ordered, esp wrt their own console.log() calls.

Given that, the idle-time prefetching seems like a decent option.

That said, I would not be averse to just killing off this particular "empty id passed to getElementById" warning.  The question is whether that would just move the time to some other string we end up needing.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Note, this will be solved with the move to the new localization API.

In the L20n world (coming to Firefox post 57 unfortunately), you'll just add an element to the console with `data-l10n-id` and we'll asynchronously localize it.

In result, the order in console will be correct, but the l10n will happen asynchronously.

The only idea I have for 57 is to do it asynchronously if console is not open?

Comment 4

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1377377 - Preload string bundles during idle time

Review commit: https://reviewboard.mozilla.org/r/165006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165006/

Comment 5

[Andreas Farre [:farre]]

I'm back from PTO the week after the next. I guess smaug could review? One pointer is to use NewIdleRunnableMethod. It's the same as NewRunnableMethod.

Comment 6

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8893944 [details]
Bug 1377377 - Preload string bundles during idle time

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165006/diff/1-2/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8893944 [details]
Bug 1377377 - Preload string bundles during idle time

https://reviewboard.mozilla.org/r/165006/#review170584

::: dom/base/nsContentUtils.cpp:3962
(Diff revision 2)
>    return NS_OK;
>  }
>  
>  /* static */
> +void
> +nsContentUtils::AsyncPreloadStringBundles()

Could you call this AsyncPrecreateStringBundles, since that is what it does.

::: dom/base/nsContentUtils.cpp:3967
(Diff revision 2)
> +nsContentUtils::AsyncPreloadStringBundles()
> +{
> +  uint32_t bundleIndex;
> +  for (bundleIndex = 0; bundleIndex < PropertiesFile_COUNT; ++bundleIndex) {
> +    nsresult rv = NS_IdleDispatchToCurrentThread(
> +      NS_NewRunnableFunction("AsyncPreloadStringBundles",

And change also this name then

::: intl/strres/nsStringBundle.cpp:49
(Diff revision 2)
>    mOverrideStrings(aOverrideStrings),
>    mReentrantMonitor("nsStringBundle.mReentrantMonitor"),
>    mAttemptedLoad(false),
>    mLoaded(false)
>  {
> +  AsyncPreloadProperties();

In principle this is bad, and might be even in real world. AsyncPreloadProperties addrefs 'this' and may also release. And release call would then delete 'this' in constructor and whoever called the constructor would get a pointer to a deleted object.

I think you should have explicit call to AsyncPreloadProperties() in nsContentUtils.

Comment 8

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8893944 [details]
Bug 1377377 - Preload string bundles during idle time

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165006/diff/2-3/

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8893944 [details]
Bug 1377377 - Preload string bundles during idle time

https://reviewboard.mozilla.org/r/165006/#review170850

::: dom/base/nsContentUtils.cpp:3965
(Diff revision 3)
>  /* static */
> +void
> +nsContentUtils::AsyncPrecreateStringBundles()
> +{
> +  uint32_t bundleIndex;
> +  for (bundleIndex = 0; bundleIndex < PropertiesFile_COUNT; ++bundleIndex) {

can't you define bundleIndex in for?

::: dom/base/nsContentUtils.cpp:3969
(Diff revision 3)
> +  uint32_t bundleIndex;
> +  for (bundleIndex = 0; bundleIndex < PropertiesFile_COUNT; ++bundleIndex) {
> +    nsresult rv = NS_IdleDispatchToCurrentThread(
> +      NS_NewRunnableFunction("AsyncPrecreateStringBundles",
> +                             [bundleIndex]() {
> +                              PropertiesFile file = static_cast<PropertiesFile>(bundleIndex);

Indentation is odd. Just one space inside lambda. Use 2.

Comment 10

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8893944 [details]
Bug 1377377 - Preload string bundles during idle time

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165006/diff/3-4/

Comment 11

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f6476d590538 -d 4ab346b62f90: rebasing 411918:f6476d590538 "Bug 1377377 - Preload string bundles during idle time r=smaug" (tip)
merging dom/base/nsContentUtils.cpp
merging dom/base/nsContentUtils.h
merging intl/strres/nsIStringBundle.idl
merging intl/strres/nsStringBundle.cpp
warning: conflicts while merging intl/strres/nsIStringBundle.idl! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 12

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8893944 [details]
Bug 1377377 - Preload string bundles during idle time

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165006/diff/4-5/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93c2fe5279fe
Preload string bundles during idle time r=smaug

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93c2fe5279fe