Closed
Bug 943924
Opened 11 years ago
Closed 11 years ago
tp5o private bytes -regression on windows xp and windows 7
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: jmaher, Assigned: bhackett1024)
Details
(Keywords: perf, regression, Whiteboard: [talos_regression][qa-])
Attachments
(1 file)
898 bytes,
patch
|
billm
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
I see a regression reported on dev.tree-management:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/XHDWPVFKjGw
this is seen in the graphs view:
http://graphs.mozilla.org/graph.html#tests=[[257,131,37],[257,131,25],[257,131,35],[257,131,33]]&sel=1382976298751,1385568298751&displayrange=30&datatype=running
This takes place during the tp5o test run (we load 49 different locally cached websites)
this is a noticeable regression on windows xp and windows 7, but an improvement on ubuntu 12.04 (32 and 64 bit).
Looking at the graphs and retriggering a few runs, the changeset that stands out is from bug 942984:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5714ab2828b
Reporter | ||
Comment 1•11 years ago
|
||
:bhacket- any thoughts if this is related to your patch?
Flags: needinfo?(bhackett1024)
Assignee | ||
Comment 2•11 years ago
|
||
The only relevant change this patch does is specify a stack size for JS worker threads. The size it specifies is 512K. Previously it used the default system thread stack size, which from bug 942984 comment 4 and http://msdn.microsoft.com/en-us/library/windows/desktop/ms686774%28v=vs.85%29.aspx is 1MB on Windows, so if anything this bug should have improved Windows memory usage.
Flags: needinfo?(bhackett1024)
Comment 3•11 years ago
|
||
Well, this sure looks like a 2-3% regression to me.
Joel, do you want to try experimentally backing it out?
Flags: needinfo?(jmaher)
Comment 4•11 years ago
|
||
If we assume that whatever change caused Ubuntu (green and orange) to jump down also caused the regression on Windows (red and blue), then it's one of these:
157706 076bf878c59a 2013-11-27 03:40 +0200 Olli
Bug 942432 - Remove nsIJSEventListener::mContext, r=bz
157707 db836ecd7746 2013-11-27 03:42 +0200 Olli
Bug 943613 - Notify JS implemented Event Target when an event listener is added / removed, r=bz
157708 551b2064b705 2013-11-26 18:06 -0800 shu
Bug 937763 - Don't emit MIR marked as emittedAtUses immediately when redefining. (r=jandem)
157709 62e94f70b2cd 2013-11-26 19:13 -0700 bhackett1024
Bug 939088 - Add a cache for fetching the names associated with ALIASEDVAR operations, r=luke.
157710 c5714ab2828b 2013-11-26 19:18 -0700 bhackett1024
Bug 942984 - Set native stack limit for JS worker threads, r=billm.
157711 3964ce61dacf 2013-11-26 21:29 -0500 ehsan
Bug 943554 - Extend the checks added in bug 941854 to all unified files; r=gps
157712 b66f95073899 2013-11-26 22:14 -0500 ehsan
Backed out changeset db836ecd7746 (bug 943613) for build bustage
157713 9849749f3623 2013-11-26 22:14 -0500 ehsan
Backed out changeset 076bf878c59a (bug 942432) for build bustage
but maybe jmaher has even more specific information than what shows up on the graph.
Reporter | ||
Comment 5•11 years ago
|
||
I have no other specific info, let me do a try run and back out some of these patches
Flags: needinfo?(jmaher)
Reporter | ||
Comment 6•11 years ago
|
||
I did a try run with the patch from bug 942984 backed out:
https://tbpl.mozilla.org/?tree=Try&rev=bfea8446c907
our private bytes did go down. Is this something we want to live with? The change is minimal, but it is showing a memory increase.
:bhackett- any more thoughts on this?
Flags: needinfo?(bhackett1024)
Assignee | ||
Comment 7•11 years ago
|
||
This patch reverts things to use the default stack size on Windows. Joel, can you see if this patch also improves the private bytes?
If so, since this actually increases the stack space available to these threads, this would indicate a bug in NSPR and/or Windows.
Flags: needinfo?(bhackett1024)
Reporter | ||
Comment 8•11 years ago
|
||
I pushed that patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=d93eac6e2e2f
the numbers are falling in line with the original numbers, it appears to fix the problem!
Is this something we can land? Need me to try anything else out?
Assignee | ||
Updated•11 years ago
|
Attachment #8343764 -
Flags: review?(wmccloskey)
Comment on attachment 8343764 [details] [diff] [review]
use default stack size on windows
Review of attachment 8343764 [details] [diff] [review]:
-----------------------------------------------------------------
Perhaps Windows doesn't commit the memory if you pass 0, but it does if you pass something else. Anyway, it's not worth investigating.
Attachment #8343764 -
Flags: review?(wmccloskey) → review+
However, once this lands, it would be great if someone from QA could verify that bug 942984 is still fixed on Windows. There's a small possibility that this patch will regress that fix.
Keywords: qawanted
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8343764 [details] [diff] [review]
use default stack size on windows
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 942984
User impact if declined: maybe increased memory usage on windows
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): low, per msdn documentation this should not rebreak bug 942984.
Attachment #8343764 -
Flags: approval-mozilla-aurora?
Comment 13•11 years ago
|
||
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Attachment #8343764 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•11 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #10)
> However, once this lands, it would be great if someone from QA could verify
> that bug 942984 is still fixed on Windows. There's a small possibility that
> this patch will regress that fix.
I'll flag that bug for testing again and follow-up there.
Keywords: qawanted
You need to log in
before you can comment on or make changes to this bug.
Description
•