GNOME Bugzilla – Bug 675663
Don't do blocking calls on main threads when collecting stats
Last modified: 2016-03-31 13:56:05 UTC
We should never do sync calls on the main thread as that may block the UI. Unfortunately we do this a lot when capturing statistics, which is particularly bad as it runs constantly, triggering UI hickups. I even regularly get a hang where the main thread gets blocked for 30 seconds until it timeouts when getting the stats. Leaving the whole desktop more or less locked up due to the keyboard grab. Attaching a patch series that fixes this.
Created attachment 213649 [details] [review] Avoid blocking call in disk stats domain.get_devices () does a blocking get_config() call, but we already have a copy of the config. Use that instead.
Created attachment 213650 [details] [review] Avoid blocking call in network stats domain.get_devices () does a blocking get_config() call, but we already have a copy of the config. Use that instead.
Created attachment 213651 [details] [review] Don't unnecessarily get info sync in stats collection The calls to is_running where causing a sync get_info() call even though we already had that information since we did an async get_info call.
Created attachment 213652 [details] [review] Turn LibvirtMachine.state from property to a method This was getting called in multiple places as if it was a cheap method, while in reality its a blocking rpc call. Sometimes it was even used multiple times within a method. The new method is called get_state_sync() to make it obvious that this is a blocking call.
Created attachment 213653 [details] [review] Track the running machine state rather than doing sync calls This means we know locally if the machine is running, paused or not running.
Created attachment 213654 [details] [review] libvirt-machine: Track the storage path so that we don't have to get it every time This is a blocking rpc call that we can easily avoid
Created attachment 213655 [details] [review] Add Util.run_in_thread This lets us easily run sync methods async
Created attachment 213656 [details] [review] Do stats collection calls async
Review of attachment 213653 [details] [review]: ::: src/libvirt-machine.vala @@ +125,3 @@ + domain.suspended.connect ( () => { running = false; paused = Signal.get_invocation_hint(domain).detail.to_string () == "paused"; } ); + domain.resumed.connect ( () => { running = true; paused = false; } ); + domain.stopped.connect ( () => { running = false; paused = false; } ); According to Zeeshan, this is problematic: https://bugzilla.gnome.org/show_bug.cgi?id=674352#c32
Patches looks fine, but they need to be rebased after Zeeshan changes from yesterday. - 1 & 2 are essentially the same fix, so it could be a single patch - 3 is probably already applied - 5 would need Zeeshan comment - 4 & 6 & 7 & 8 ack, although why use pointers? is that a vala bug?
Review of attachment 213649 [details] [review]: ACK
Review of attachment 213650 [details] [review]: ACK
Review of attachment 213651 [details] [review]: Already fixed in trunk.
Review of attachment 213652 [details] [review]: This is also already taken care of.
Pointers are needed, because "ref" is not supported for async methods in vala.
Review of attachment 213653 [details] [review]: ::: src/libvirt-machine.vala @@ +125,3 @@ + domain.suspended.connect ( () => { running = false; paused = Signal.get_invocation_hint(domain).detail.to_string () == "paused"; } ); + domain.resumed.connect ( () => { running = true; paused = false; } ); + domain.stopped.connect ( () => { running = false; paused = false; } ); Yeah, it will at least break boxes for me. :) Also, we don't need separate bool props as we can simply update existing state prop on state changes.
Review of attachment 213654 [details] [review]: ACK but please try to keep the commit short log < 50 chars
Review of attachment 213655 [details] [review]: Apart from this nitpicks, good stuff! ::: src/util.vala @@ -438,0 +438,5 @@ + + public delegate void RunInThreadFunc () throws GLib.Error; + public async void run_in_thread (RunInThreadFunc func) throws GLib.Error { ... 2 more ... redundant space between 2 '(' @@ -438,0 +438,17 @@ + + public delegate void RunInThreadFunc () throws GLib.Error; + public async void run_in_thread (RunInThreadFunc func) throws GLib.Error { ... 14 more ... I'd put a newline before & after 'yield' here but its up to you.
Review of attachment 213656 [details] [review]: ::: src/libvirt-machine.vala @@ -157,3 +157,3 @@ } - private void update_io_stat (DomainInfo info, ref MachineStat stat) { + private async void update_io_stat (DomainInfo info, MachineStat *stat) { As elmarco, pointed out there is no need for use of pointers. A 'ref' param perhaps? @@ -166,3 +166,3 @@ return; - stat.disk = disk.get_stats (); + yield run_in_thread ( () => { I suggest to instead add async variants to these libvirt-glib calls. That would be consistent with how we have been solving these kind of issues and benefit other libvirt-glib apps too.
Review of attachment 213656 [details] [review]: ::: src/libvirt-machine.vala @@ +157,3 @@ } + private async void update_io_stat (DomainInfo info, MachineStat *stat) { Ref isn't supported on async functions. @@ +166,3 @@ return; + yield run_in_thread ( () => { I agree that libvirt-glib should have these, but it should be easy to switch boxes to that when its supported.
Created attachment 213664 [details] [review] Track the running machine state rather than doing sync calls This means we know locally if the machine is running, paused or not running.
Updated the event patch
Created attachment 213665 [details] [review] Make use of domain signals to keep track of it's state
Created attachment 213673 [details] [review] Alternative event patch This patch doesn't use DomainState as that is not what you get from following the events.
Review of attachment 213673 [details] [review]: Looks good but wonder if it would be a good idea to put this new enum and is_running impl to base Machine class?
Created attachment 213676 [details] [review] Add MachineState enum and property to Machine, implement is_running over it This means we can track the state without having to do sync calls.
Review of attachment 213676 [details] [review]: Looks good. Just one nitpick about commit summary again: Please move 'and implement is_running over it' part to description and 'and' -> '&' to fit summary in ~50 cols.
commited.
there is a regression that has been introduced the screenshots are grayed in initial startup even if the VM is running
Thats likely from is_running moving to events, as thats what triggers that. Will look into it.
Created attachment 213735 [details] [review] Init Machine.state in constructor, not later We need this early on to see whether to show the screenshot as running or not on startup.
Review of attachment 213735 [details] [review]: ack, thanks
This bug is fixed now?
(In reply to comment #33) > This bug is fixed now? I assume the answer is 'yes' since all patches have been committed and I dont see any pending issues.. Feel free to correct.