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

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM
RESOLVED FIXED
2 months ago
11 days ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: bytesized)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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)
Boris, do you know what should do here? Some sort of caching?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)

Updated

a month ago
Whiteboard: [qf] → [qf:p1]
(Assignee)

Updated

a month ago
Assignee: nobody → ksteuber
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 hidden (mozreview-request)
(Assignee)

Updated

14 days ago
Attachment #8893944 - Flags: review?(afarre)
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.
(Assignee)

Updated

14 days ago
Attachment #8893944 - Flags: review?(afarre)
Comment hidden (mozreview-request)
(Assignee)

Updated

14 days ago
Attachment #8893944 - Flags: review?(bugs)

Comment 7

12 days ago
mozreview-review
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.
Attachment #8893944 - Flags: review?(bugs) → review-
Comment hidden (mozreview-request)

Comment 9

11 days ago
mozreview-review
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.
Attachment #8893944 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)

Comment 11

11 days ago
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 hidden (mozreview-request)

Comment 13

11 days ago
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93c2fe5279fe
Preload string bundles during idle time r=smaug

Comment 14

11 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/93c2fe5279fe
Status: NEW → RESOLVED
Last Resolved: 11 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.