Closed Bug 1377377 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: bytesized)

References

Details

(Keywords: perf)

Attachments

(1 file)

Bug 1377377 - Preload string bundles during idle time
59 bytes, text/x-review-board-request
smaug
: review+
Details 
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)
Whiteboard: [qf] → [qf:p1]
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?
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.
Attachment #8893944 - Flags: review?(afarre)
Attachment #8893944 - Flags: review?(bugs)
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 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+
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)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93c2fe5279fe
Preload string bundles during idle time r=smaug
https://hg.mozilla.org/mozilla-central/rev/93c2fe5279fe
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: