Re: [chromium-dev] Singleton shenanigans in base/time_win.cc causing problems (not the first time)
On Fri, Nov 20, 2009 at 23:34, Darin Fisher da...@chromium.org wrote: We could define a function that must be called before you can use code in base/. You could add a call to this everywhere that we currently create the AtExitManager. Or, maybe we could combine those somehow. Thanks for the ideas! That inspired me to choose current solution ( http://codereview.chromium.org/431008) after experimenting with some other choices. I'm thinking about removing the other Singletons in time_win.cc. They're not so evil, but their presence might encourage creating more Singletons unnecessarily. And on Vista, we have better time calls, which don't have the rollover issue etc. -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Singleton shenanigans in base/time_win.cc causing problems (not the first time)
I think we should have a list of low-level functionality which we just never cleanup. For the items you listed, I think you should leak them all. Trying to cleanup these items creates complicated code and ultimately won't run any better and possibly slower. Mike On Fri, Nov 20, 2009 at 1:44 PM, Paweł Hajdan Jr. phajdan...@chromium.orgwrote: Do you have some idea how to get rid of the Singletons in base/time_win.cc? They don't play very well with base::SystemMonitor, MessageLoop, and test code. Here's the scenario we're hitting right now (in browser_tests): 1. HighResolutionTimerManager is created to enable high resolution timer unconditionally. An AtExit callback is created to destroy it. 2. The main MessageLoop is created. Its LazyInstance initializes a ThreadLocalPointer, which gets assigned the pointer to the MessageLoop. An AtExit callback is registered to reset the LazyInstance to the initial state. 3. BrowserMain enables monitoring SystemMonitor in HighResolutionTimerManager. 4. Which makes HighResolutionTimerManager register as a SystemMonitor observer. If everything seems OK so far, you're almost right. It's perfectly fine when browser runs, but has surprising special effects when test code runs. Here's what happens: 1. MessageLoop is destroyed. Its ThreadLocalPointer is set to NULL. 2. AtExit callbacks start to execute. 3. HighResolutionTimerManager is destroyed first. Its StopMonitoring method is called from dtor, which in turn touches MessageLoop::current(), which touches LazyInstance and makes it re-initialize itself, which registers AtExit callbacks which causes a DCHECK failure, because we're already processing AtExit callbacks. Don't worry too much if the explanation above is unclear (I'm afraid it is). The point is, it's not the first time that a nasty bug happens with SystemMonitor and the time_win.cc Singletons. Earlier it was different, so it's not so much important in what particular way it failed now. I'd really love to get rid of these Singletons. Do you have any ideas? We have two things here: 1) NowSingleton which keeps track of Windows time rollover. This might be harder to remove, but isn't causing too many problems. 2) HighResolutionTimerManager is causing problems. It was previously part of NowSingleton. I extracted the most evil part of it, and this is the current HRTM. I'm thinking about hardwiring it with the SystemMonitor or making it a Leaky Singleton maybe... -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Singleton shenanigans in base/time_win.cc causing problems (not the first time)
On Sun, Nov 22, 2009 at 4:28 PM, Mike Belshe mbel...@google.com wrote: I think we should have a list of low-level functionality which we just never cleanup. For the items you listed, I think you should leak them all. Trying to cleanup these items creates complicated code and ultimately won't run any better and possibly slower. +1 for leaking a much as possible on shutdown. We should make sure we shut down anything that can write data to disk, and then just kill the process. Right now I suspect we clean up much more stuff in the browser process than we have to. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Singleton shenanigans in base/time_win.cc causing problems (not the first time)
It's really about the unit tests, not in chromium. I guess we could probably leak the singleton in the unit tests too on each reset. Pawel, what do you think? Note to all the static local makers: you create an implicit atexit() each time... Use a leaky singleton instead. M-A On Sun, Nov 22, 2009 at 7:47 PM, Peter Kasting pkast...@google.com wrote: On Sun, Nov 22, 2009 at 4:28 PM, Mike Belshe mbel...@google.com wrote: I think we should have a list of low-level functionality which we just never cleanup. For the items you listed, I think you should leak them all. Trying to cleanup these items creates complicated code and ultimately won't run any better and possibly slower. +1 for leaking a much as possible on shutdown. We should make sure we shut down anything that can write data to disk, and then just kill the process. Right now I suspect we clean up much more stuff in the browser process than we have to. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Singleton shenanigans in base/time_win.cc causing problems (not the first time)
We could define a function that must be called before you can use code in base/. You could add a call to this everywhere that we currently create the AtExitManager. Or, maybe we could combine those somehow. I don't think lazy initialization of stuff like this is all that valuable. It is complexity for no real benefit. -Darin On Fri, Nov 20, 2009 at 1:44 PM, Paweł Hajdan Jr. phajdan...@chromium.orgwrote: Do you have some idea how to get rid of the Singletons in base/time_win.cc? They don't play very well with base::SystemMonitor, MessageLoop, and test code. Here's the scenario we're hitting right now (in browser_tests): 1. HighResolutionTimerManager is created to enable high resolution timer unconditionally. An AtExit callback is created to destroy it. 2. The main MessageLoop is created. Its LazyInstance initializes a ThreadLocalPointer, which gets assigned the pointer to the MessageLoop. An AtExit callback is registered to reset the LazyInstance to the initial state. 3. BrowserMain enables monitoring SystemMonitor in HighResolutionTimerManager. 4. Which makes HighResolutionTimerManager register as a SystemMonitor observer. If everything seems OK so far, you're almost right. It's perfectly fine when browser runs, but has surprising special effects when test code runs. Here's what happens: 1. MessageLoop is destroyed. Its ThreadLocalPointer is set to NULL. 2. AtExit callbacks start to execute. 3. HighResolutionTimerManager is destroyed first. Its StopMonitoring method is called from dtor, which in turn touches MessageLoop::current(), which touches LazyInstance and makes it re-initialize itself, which registers AtExit callbacks which causes a DCHECK failure, because we're already processing AtExit callbacks. Don't worry too much if the explanation above is unclear (I'm afraid it is). The point is, it's not the first time that a nasty bug happens with SystemMonitor and the time_win.cc Singletons. Earlier it was different, so it's not so much important in what particular way it failed now. I'd really love to get rid of these Singletons. Do you have any ideas? We have two things here: 1) NowSingleton which keeps track of Windows time rollover. This might be harder to remove, but isn't causing too many problems. 2) HighResolutionTimerManager is causing problems. It was previously part of NowSingleton. I extracted the most evil part of it, and this is the current HRTM. I'm thinking about hardwiring it with the SystemMonitor or making it a Leaky Singleton maybe... -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Singleton shenanigans in base/time_win.cc causing problems (not the first time)
On Mon, Nov 23, 2009 at 02:22, Marc-Antoine Ruel mar...@google.com wrote: It's really about the unit tests, not in chromium. I guess we could probably leak the singleton in the unit tests too on each reset. Pawel, what do you think? Note to all the static local makers: you create an implicit atexit() each time... Use a leaky singleton instead. That was my first though as well, but it won't fix the problem really. If a Singleton touches a MessageLoop, we're going to have bad problems in tests (because we create and re-instantiate it for different tests, and some tests don't have MessageLoop at all). And SystemMonitor does, and has to. SystemMonitor makes one assumption that we can't really where it is currently: that one MessageLoop is always running through its lifetime. I have a patch ready (have to submit it yet) that moves SystemMonitor to chrome/common, as well as HighResolutionTimerManager, and de-Singletonizes them. They have to be instantiated where currently SystemMonitor::Enable is called. I verified that we don't get the bad problems that we currently have with that. If it doesn't convince you... SystemMonitor was a LeakySingleton. And it still caused problems because of its assumption that the same MessageLoop exists through its lifetime. I think that moving it to a place where we can make that assumption valid is the proper fix. Another (slightly weird maybe) solution would be to have a per-thread SystemMonitor, lazily created. Then we could just use ObserverList, which doesn't use MessageLoop. But IMHO it is more complex than the proposed solution above, and may have its own problems. What do you think? -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
