Closed
Bug 1264896
Opened 9 years ago
Closed 9 years ago
GC calls make page loading extremely slow
Categories
(Core :: JavaScript: GC, defect)
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)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Reporter | ||
Comment 1•9 years ago
|
||
Apparently, this only happens when browsing in e10s mode. Legacy mode is fine.
Updated•9 years ago
|
Component: Untriaged → JavaScript: GC
Product: Firefox → Core
Comment 2•9 years ago
|
||
I can confirm the problem.
regression range:
Last OK: http://hg.mozilla.org/integration/mozilla-inbound/rev/cd77e25dd600
First Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/1e6cb22a8971
Bug 1257164, Bug 888969 and Bug 1259877 in this range.
Comment 3•9 years ago
|
||
The problem happens only when NoScript addon is enabled.
Updated•9 years ago
|
tracking-e10s:
--- → ?
Comment 5•9 years ago
|
||
As far as I tested in my build, the problem disappeared after I reverted two commits in bug 888969.
Assignee | ||
Comment 6•9 years ago
|
||
(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!)
Comment 7•9 years ago
|
||
(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
Assignee | ||
Comment 8•9 years ago
|
||
Glorious. Patching now.
Assignee | ||
Comment 9•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → jwalden+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 10•9 years ago
|
||
Try-push with a few other things mixt in:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b37ddec5f911
Flags: needinfo?(jwalden+bmo)
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
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.
Comment 13•9 years ago
|
||
(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.
Updated•9 years ago
|
Blocks: e10s-addons
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+
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
(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.
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)
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)
Assignee | ||
Comment 22•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jwalden+bmo)
Updated•9 years ago
|
status-firefox48:
--- → affected
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+
Comment 25•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•