Closed Bug 943924 Opened 7 years ago Closed 7 years ago

tp5o private bytes -regression on windows xp and windows 7

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jmaher, Assigned: bhackett1024)

Details

(Keywords: perf, regression, Whiteboard: [talos_regression][qa-])

Attachments

(1 file)

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
:bhacket- any thoughts if this is related to your patch?
Flags: needinfo?(bhackett1024)
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)
Well, this sure looks like a 2-3% regression to me.

Joel, do you want to try experimentally backing it out?
Flags: needinfo?(jmaher)
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.
I have no other specific info, let me do a try run and back out some of these patches
Flags: needinfo?(jmaher)
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)
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)
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?
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
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?
https://hg.mozilla.org/mozilla-central/rev/61cda1f28ce4
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Attachment #8343764 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [talos_regression] → [talos_regression][qa-]
(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.