Closed Bug 1264896 Opened 4 years ago Closed 4 years ago

GC calls make page loading extremely slow

Categories

(Core :: JavaScript: GC, defect)

48 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox47 --- unaffected
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: m.danai, Assigned: Waldo)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Attached image gcff.PNG
User Agent: Mozilla/5.0 (masking-agent; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20160414030247

Steps to reproduce:

Upon loading any web page, the browser freezes just before loading the actual page content. After a moment, it comes back to life.


Actual results:

If you watch the performance graph, you will notice 3 GC calls that delay the page rendering by a few seconds with freezing involved.


Expected results:

No extra delay or unbugged GC calls.
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Apparently, this only happens when browsing in e10s mode. Legacy mode is fine.
Component: Untriaged → JavaScript: GC
Product: Firefox → Core
The problem happens only when NoScript addon is enabled.
NI Waldo per comment 2.
Flags: needinfo?(jwalden+bmo)
As far as I tested in my build, the problem disappeared after I reverted two commits in bug 888969.
(In reply to Toshihiro Yamada from comment #5)
> As far as I tested in my build, the problem disappeared after I reverted two
> commits in bug 888969.

Any chance you could check which of the two commits is at fault?  (My guess is the nsIRemoteTagService.getRemoteObjectTag patch.  If so, looks like C++ification is in order!)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> (In reply to Toshihiro Yamada from comment #5)
> > As far as I tested in my build, the problem disappeared after I reverted two
> > commits in bug 888969.
> 
> Any chance you could check which of the two commits is at fault?  (My guess
> is the nsIRemoteTagService.getRemoteObjectTag patch.  If so, looks like
> C++ification is in order!)

Yes, nsIRemoteTagService.getRemoteObjectTag patch is a cause of the problem.

revert only https://hg.mozilla.org/mozilla-central/rev/498e330e857d : hangs happens
revert only https://hg.mozilla.org/mozilla-central/rev/fe3aba52a965 : no problem
Glorious.  Patching now.
Bill, this is a followup to bug 888969's attachment 8740192 [details] [diff] [review], because apparently the try-catches and other nonsense in that attachment slow things down a bunch.  Shocker.

This is mostly a stupid, obvious patch for a thing that has no business being XPCOM and generic (pun intended) and contract-overridable, &c.  But, can IPC depend on nsIDocShellTreeItem/nsIDOMDocument?  There's already a dom dependency in WrapperOwner.cpp, so this seems like it should be kosher.  Right?

Haven't tryservered yet, but this passes test_cpows.xul, which seems a good enough smoketest for reviewing.
Attachment #8744157 - Flags: review?(wmccloskey)
Assignee: nobody → jwalden+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Try-push with a few other things mixt in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b37ddec5f911
Flags: needinfo?(jwalden+bmo)
I'm testing the patch now.
It seems that browser-hang doesn't happen.

But, until now, I encountered application exceptions on plugin-container.exe several times.
(Probably, access to invalid address is happened)
I'll try to check whether the exception is related to the patch or not.
Try results appear green, so unless there's some crash here (seems doubtful unless bz led me astray with the invocation to perform), this is good to review.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> Try results appear green, so unless there's some crash here (seems doubtful
> unless bz led me astray with the invocation to perform), this is good to
> review.

Yes, I tested with newer changeset, and no problem with this build.
Comment on attachment 8744157 [details] [diff] [review]
Kill off nsIRemoteTagService and do what it does, in its sole caller, in far-faster C++

Review of attachment 8744157 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this is okay. The purpose of doing it in JS was to make it easier to modify and add cases, but we haven't really done that.

I'm surprised this code is slowing anything down. It's only called when we're already doing CPOW IPC, which I would expect to be much slower than whatever XPConnect stuff we have to do here. Some add-on must be doing something really wacky--creating CPOWs and then not using them maybe.
Attachment #8744157 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/fc1b655d1080
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Is it possible to get this uplifted to 48? It looks like this is affecting other NoScript users as well (See Bug 1058542 comment 74 and Bug 1058542 comment 75).

Thanks!
Flags: needinfo?(jwalden+bmo)
(In reply to Bill McCloskey (:billm) from comment #14)
> I'm surprised this code is slowing anything down. It's only called when
> we're already doing CPOW IPC, which I would expect to be much slower than
> whatever XPConnect stuff we have to do here. Some add-on must be doing
> something really wacky--creating CPOWs and then not using them maybe.

Even light browsing results in dozens of "unsafe CPOW usage" messages in the browser console due to NoScript. Not sure if that counts.
Attached patch mozilla-aurora backport (obsolete) — Splinter Review
I pair programmed this with :Waldo and fixed a failing hunk that would prevent landing safely on mozilla-aurora.
Attachment #8749904 - Flags: review?(jwalden+bmo)
Attached patch mozilla-aurora backport (obsolete) — Splinter Review
Updated patch with user and commit messages.
Attachment #8749904 - Attachment is obsolete: true
Attachment #8749904 - Flags: review?(jwalden+bmo)
Attachment #8749906 - Flags: review?(jwalden+bmo)
On second thought, Waldo mentions only this change should suffice for a backport.
Attachment #8749906 - Attachment is obsolete: true
Attachment #8749906 - Flags: review?(jwalden+bmo)
Attachment #8749907 - Flags: review?(jwalden+bmo)
Comment on attachment 8749907 [details] [diff] [review]
mozilla-aurora backport v2

Review of attachment 8749907 [details] [diff] [review]:
-----------------------------------------------------------------

Approval Request Comment
[Feature/regressing bug #]: one particular portion of bug 888969 that landed before the branch
[User impact if declined]: slowness of some operations with e10s running
[Describe test coverage new/current, TreeHerder]: none -- if this is wrong, e10s of browser tabs is probably going to be faily pretty obviously
[Risks and why]: minimal -- the new code is pretty simple
[String/UUID change made/needed]: no interface/string changes made

(In reply to :Cykesiopka from comment #17)
> Is it possible to get this uplifted to 48?

Ugh, right, I meant to do that and then forgot.  :-(

The original patch makes extra changes to remove code that's dead after IPC performs nsIRemoteTagService's operations itself.  But in doing so it removes existing interfaces and such -- not ideal or necessary for a backport.  This patch is the smallest thing that should do the trick, without removing an interface that someone might be using (super-unlikely, but no reason to take the risk, or to argue it for backporting purposes ;-) ).
Attachment #8749907 - Flags: review?(jwalden+bmo)
Attachment #8749907 - Flags: review+
Attachment #8749907 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jwalden+bmo)
This was confusing at first but I can see from https://bugzilla.mozilla.org/show_bug.cgi?id=888969#c17 that some stuff landed in 48.
Comment on attachment 8749907 [details] [diff] [review]
mozilla-aurora backport v2

Fix for recent regression, should improve performance. Please uplift to aurora.
Attachment #8749907 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.