Changes to speed up qvm-ls - #36
Open
qubesuser wants to merge 6 commits into
Open
Conversation
qubesuser
force-pushed
the
fast_qvm_ls
branch
5 times, most recently
from
November 10, 2017 15:30
949275c to
d8246fb
Compare
This is much more efficient.
This makes it get all the data with a single qubesd call.
If qubesd returns a label name, we can just assume it's valid. This makes qvm-ls take only one qubesd call.
It's slow and unnecessary
Tools should be written to be fast enough that spinners don't become necessary.
qubesuser
force-pushed
the
fast_qvm_ls
branch
from
November 10, 2017 15:32
d8246fb to
671150d
Compare
marmarek
reviewed
Nov 10, 2017
| vm_state = vm_list_info[0].strip().partition('state=')[2].split(' ')[0] | ||
| return vm_state | ||
|
|
||
| if not self._state or force: |
Member
There was a problem hiding this comment.
Ouch, this will break all the long-running processes using vm.is_running() or such. One example is qvm-shutdown --wait. IMO caching of VM state should be opt-in, not opt-out.
marmarek
requested changes
Nov 10, 2017
marmarek
left a comment
marmarek
left a comment
Member
There was a problem hiding this comment.
I haven't reviewed all of this properly. Will do when we agree on the final backend side shape. But at least separate GetAll from GetAllData support - currently parts of the latter are in commit related to the former.
| except qubesadmin.exc.QubesDaemonNoResponseError: | ||
| raise qubesadmin.exc.QubesPropertyAccessError(key) | ||
|
|
||
| self._set_item(key, value, False) |
Member
There was a problem hiding this comment.
This is fragile. Especially for long-living processes - anyone else can modify value in the meantime. But also any extension is free to modify value in event handler, including event caused by this very property set.
So, if do this at all, at least make sure the application also receive events. Or at least have it opt-in (and explicitly enable, only in short-lived processes, like some qvm-* tools).
| hideseq = '\n' | ||
|
|
||
| self.stream.write(hideseq) | ||
| self.stream.flush() |
Member
|
@marmarek Are we still interested in adding Are there other parts of this old PR that we are still interested in ? (if so I can try to take over) |
Member
|
Hm, at this point, I guess the |
Contributor
|
Simple test to show hot areas: Amount of qubes: Package version: qvm-ls output
cProfile
pyinstrument
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the client part of a bunch of changes that get qvm-ls to around 200ms run time.
Warning: I haven't tested them much, and they change fundamental code, so they probably need fixes.
In particular, all properties are now cached by default, so anything that expects to see changes is now broken and needs to be fixed.
This makes use of the new APIs introduced in QubesOS/qubes-core-admin#165, and also contains a bunch of optimizations of the Python code.