On 5/3/2010 4:15 PM, Nobody wrote:
> Regression: Ts Shutdown, Cold MAX Dirty Profile increase 5.80% on MacOSX 10.5.8 Firefox
> Previous results:
> 108.0 from build 20100503030614 of revision 83c887dff0da at 2010-05-03 04:28:00 on talos-r3-leopard-037 run # 0
> New results:
> 114.263 from build 20100503034701 of revision 358113b3642e at 2010-05-03 04:09:00 on talos-r3-leopard-027 run # 0
> http://mzl.la/a1lpv9
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c887dff0da&tochange=358113b3642e
> _______________________________________________
> dev-tree-management mailing list
> dev-tree-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-tree-management
I hope we're not parsing much HTML at shutdown. :-/
I think that Tshutdown, being basically an I/O test AFAICT, is just
noisier than normal for our tests, and confusing the regression
detector. But I cannot prove it!
Mike
The only thing I could think of is that we might have another thread to
spin down during shutdown.
/ Jonas
IMO, this should be backed out until we figure out what's up.
(side note: this also landed with some nice performance wins)
Cheers,
Shawn
Were the wins on something more important than 5% on Tshutdown? If so
-- and it's hard to imagine a win that isn't, IMO -- then it should
stay.
Tshutdown really just isn't that important, and that we're reporting
it is mostly an artifact of what we did to avoid counting it in Ts, if
memory serves. We shouldn't let the fact that it's tracked on graph
server make it a goal in its own right.
Mike
> Tshutdown really just isn't that important, and that we're reporting
> it is mostly an artifact of what we did to avoid counting it in Ts, if
> memory serves. We shouldn't let the fact that it's tracked on graph
> server make it a goal in its own right.
I beg to differ. We have lots of users who hit the "firefox is already
running" dialog when simply restarting their browser. Yes, Tp4 shutdown
is noisy as heck and I wouldn't really trust it. However, Ts shutdown
is about as stable as Ts proper. There is a clear regression on 10.5.8
and a less clear one on linux 64. Windows 7 and XP look alright and
there might be a win on 10.6.2. I can't tell with linux; there may be a
small regression.
I'm advocating that we back it out and identify what extra work we are
doing at shutdown. Just taking a regression without making some effort
to understand it seems less than ideal to me.
Cheers,
Shawn
Cheers,
Shawn
Mike
On May 3, 2010 5:37 PM, "Shawn Wilsher" <sdw...@mozilla.com> wrote:
On 5/3/2010 2:16 PM, Mike Shaver wrote:
>
> Were the wins on something more important than 5% on Tsh...
5% Tp4 (Linux only)
3% Tp4 Private bytes (Linux64 only)
1.72% Dromaeo (10.5 only)
> Tshutdown really just isn't that important, and that we're reporting
> it is mostly an artifact ...
I beg to differ. We have lots of users who hit the "firefox is already
running" dialog when simply restarting their browser. Yes, Tp4 shutdown is
noisy as heck and I wouldn't really trust it. However, Ts shutdown is about
as stable as Ts proper. There is a clear regression on 10.5.8 and a less
clear one on linux 64. Windows 7 and XP look alright and there might be a
win on 10.6.2. I can't tell with linux; there may be a small regression.
I'm advocating that we back it out and identify what extra work we are doing
> Looks like the HTML5 parser change - expected?
No, the regression was not expected.
As sicking says, the only obvious shutdown path difference is that there's one more thread to shut down.
I'll run the shutdown test locally on a Mac with printfs in interesting places.
> On 5/3/2010 4:15 PM, Nobody wrote:
> > Regression: Ts Shutdown, Cold MAX Dirty Profile increase 5.80% on
> MacOSX 10.5.8 Firefox
> > Previous results:
> > 108.0 from build 20100503030614 of revision 83c887dff0da at
> 2010-05-03 04:28:00 on talos-r3-leopard-037 run # 0
> > New results:
> > 114.263 from build 20100503034701 of revision 358113b3642e
> at 2010-05-03 04:09:00 on talos-r3-leopard-027 run # 0
> > http://mzl.la/a1lpv9
> >
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c887dff0da&tochange=358113b3642e
--
Henri Sivonen
hsiv...@iki.fi
http://hsivonen.iki.fi/
> "Mike Beltzner" <belt...@mozilla.com> wrote:
>
> > Looks like the HTML5 parser change - expected?
>
> No, the regression was not expected.
>
> As sicking says, the only obvious shutdown path difference is that
> there's one more thread to shut down.
The thread shutdown time isn't even measurable at the millisecond-resolution, so that's not the problem.
> I'll run the shutdown test locally on a Mac with printfs in
> interesting places.
On my MacBook (Core 2 Duo, Snow Leopard), ts shutdown is worse with the old parser than with the new parser, so I'm unable to reproduce the regression locally.
The most interesting thing I discovered was this:
After nsAppStartup::Quit has been called, no stream-originating HTML parsing takes place and no innerHTML setters run. However, document.write() is called once with
"\n\n"Startup time = 1375 ms<br>" as the argument and there's no associated document.close() call.
This comes from startup.html. In the onload handler, there's this line:
document.write('\n\nStartup time = ' + startupTime + ' ms<br>');
This line is misindented, which suggests it's debug code that should not have made it into production. In any case, calling document.write() from the onload handler is improper, because by the time onload fires, the parser is done, so the document.write() implies document.open() and starts another parse.
I suggest removing the document.write() line from the harness. I don't know if doing that would fix the regression, though.
Cheers,
Shawn
> I suggest removing the document.write() line from the harness. I don't
> know if doing that would fix the regression, though.
Filed as https://bugzilla.mozilla.org/show_bug.cgi?id=563899