server_x11 performance boost

[mzizzi]

Following discussion in #103

Performance improvements for get_context in server_x11.py via libxdo/xlib bindings.

This implementation should be much quicker and backwards compatible with the current get_context code.

On my ubuntu based machine I had to install the libX11-dev package to use the x11 bindings. ./server/linux_x11/requirements.txt includes added dependencies for these updates.

Install them with:

$ pip install -r server/linux_x11/requirements.txt

On my machine the boost in performance looks like this:
New implementation takes about 0.000994 seconds per get_context call.
Old implementation take about 0.013734 seconds per get_context call.

This leaves us with a 13x performance boost. The slowest part here is querying the additional x11 properties that we sometimes use. The improved implementation would be roughly 2x faster than it already is if we drop querying X11 for the following props:
WM_WINDOW_ROLE, _NET_WM_WINDOW_TYPE, _NET_WM_PID, WM_LOCALE_NAME, WM_CLIENT_MACHINE

Please give the branch a try and let me know what you think.

[mzizzi]

I'd like to hold off on merging this into aenea until rshk/python-libxdo#3 is merged.

[mzizzi]

@djr7C4 would you mind giving this branch a go and sharing your thoughts? You'll have to install the deps listed in server/linux_x11/requirements.txt as well as the libX11-dev linux package. It should be 100% compatible with the current implementation. Hopefully the only perceived difference is speed :-)

[djr7C4]

@mzizzi I will when I have a chance. Lately, however, I've been busy with job applications and have lost my voice due to being sick for the last two weeks so I am not sure when I will be able to get to it.

It does sound like a good change!

[mzizzi]

This branch should still be ready to rock. @calmofthestorm @djr7C4 is there anything you would like to see to help prove this one out?

[calmofthestorm]

Hi, sorry for the delay. I tried this out, and it fails my basic test script (test-client.py). The issue does not seem to be related to RPC, when I put the failing call into the getcontext mode in main, I get the following backtrace when I call move_mouse(0, 0).

Traceback (most recent call last):
File "server_x11.py", line 570, in
move_mouse(0, 0)
File "server_x11.py", line 456, in move_mouse
geo = get_geometry()
File "server_x11.py", line 225, in get_geometry
window_location = libxdo.get_window_location(window_id)
File "build/bdist.linux-x86_64/egg/xdo/init.py", line 548, in get_window_location
TypeError: byref() argument must be a ctypes instance, not '_ctypes.PyCPointerType'

The test script methods that fail are:
test_click_mouse(distobj)
test_move_mouse(distobj)
test_mouse_drag(distobj)
test_pause(distobj)

I suspect the above backtrace is the root cause of the three mouse-related failures.

I believe the pause test is just due to renaming amount -> millis. I'm fine with the rename in principle, but it will break anything that calls it using the kw style. Given that this is somewhat internal and not (typically) called by grammars that's fine, we just need to update the test script.

Thanks again for doing this. I'm sorry it's taken me so long to get back to you. If you can get these fixed I'll try to take another look soon.

[calmofthestorm]

By the way, aside from these issues it looks good. I want to dig a bit deeper into the timing stuff and make sure my tests thoroughly exorcise the API before merging, but I'm excited at how big a win this will be for latency.

[mzizzi]

Good catch on the get_geometry bug. I'll dig into https://github.com/rshk/python-libxdo a bit more and figure out what's going on there. On that note we can probably write a solid test suite by following the same method used by the python-libxdo project.

[calmofthestorm]

A test suite like that would be great, but I won't insist on it to merge this since we don't currently have a solid test suite for the server.

I'd love to get aenea off manual testing (there are a few unit tests for core pieces that were well isolated I guess), but it's hard to imagine a harder sort of project to test, and I feel like our limited time for the project is better spent elsewhere, especially given all its users are technical.

Honestly what scares me more than regressions in the (seldom modified) server is regressions in grammars, but I'm not even sure how to start testing that -- mocking out Dragon/Dragonfly would be extremely challenging. It might be possible to do an integration test using Dragonfly's testing capabilities (iirc it has a way to simulate speech), against a mocked-out server.

[mzizzi]

@calmofthestorm I submitted a fix for the bugs you ran into. I'd actually already fixed the underlying issue in python-libxdo but forgot to bump the dep version in requirements.txt. I'd love to run test_servery_x11.py to verify the fix but it seems that whatever "util" scripts the tests rely on are both gitignore'd and missing.

[calmofthestorm]

I tried installing the requirements you specify but got this error:

alexr@oak:~/up2date/aenea/server/linux_x11$ pip install -r requirements.txt 
Downloading/unpacking psutil==3.0.0 (from -r requirements.txt (line 1))
  Downloading psutil-3.0.0.tar.gz (240kB): 240kB downloaded
  Running setup.py (path:/tmp/pip-build-1PPjyb/psutil/setup.py) egg_info for package psutil

    warning: no previously-included files matching '*' found under directory 'docs/_build'
Downloading/unpacking python-xlib==0.15rc1 (from -r requirements.txt (line 2))
  Could not find any downloads that satisfy the requirement python-xlib==0.15rc1 (from -r     requirements.txt (line 2))
Cleaning up...
No distributions at all found for python-xlib==0.15rc1 (from -r requirements.txt (line 2))
Storing debug log for failure in /home/alexr/.pip/pip.log

What am I missing? If this version of python-xlib is not readily available via pip yet I'd prefer to hold off merging until it is to avoid breaking users. I don't want to ask people to install manually a version of dependencies as a regression.

Re testing -- test_server_x11.py is just mocks on the server, it doesn't actually test real commands, and has been broken for awhile (too tied to the implementation details of the server). I decided unit tests were not worth the trouble for the server since all it does is delegate to xdotool (or libxdo).

Also pause is still broken.

Thanks! Sorry to be so picky here, I just want to make sure we don't break anything for people who rely on it.

Try test-client.py. It has no dependencies except config.py and jsonrpclib.

[mzizzi]

My apologies. python-xlib actually isn't available via pypi and I'll need to fix that dependency. It is pip-installable, however. We can use an svn target in requirements.txt.

I'm working with python-libxdo project to get a new release in pypi that we can use.

As far as the millis vs amount argument I'll make the change back to amount. The aenea client and other server implementations that specifically use amount instead of millis and I don't want to break that.

I'll be back with those changes in the near future.

[calmofthestorm]

Thanks!

[mzizzi]

I took the time to address the easy fixes this morning. I'll take the time to tackle the amount/millis stuff once we get #114 figured out. Ultimately I see this PR getting closed and re-opened against master after #114 goes in.

I know I've been picky here and on #114 but I really do appreciate the effort you've put into both.

You've given nothing but solid criticism and that leads to better software.

[mzizzi]

As of now you can start server_x11.py with either the xdotool or libxdo backend. Running with no args acts identically to how the server currently works.

$ python server/linux_x11/server_x11.py --help
usage: server_x11.py [-h] [--daemon] [--input {xdotool,libxdo}]

Aenea Linux X11 Server

optional arguments:
  -h, --help            show this help message and exit
  --daemon              If provided the server runs in the background.
  --input {xdotool,libxdo}
                        Aenea Server Input Method. Providing the default,
                        "xdotool" will make the server shell out to the
                        xdotool program to emulate input. "libxdo" will cause
                        the server to make calls to the xdo library.

[calmofthestorm]

Thank you! Everything worked great with the xdotool client. When I ran the newly added integration_test.py with the libxdo version I got multiple failures. Does integration_test.py pass for you when using libxdo? If not I can help you reproduce and/or investigate further myself.

[mzizzi]

I am able to successfully run the test suite against the libxdo impl after the bugfix from 1d8c026.

If you still have issues running the test suite then you're probably using an older version of python-libxdo from when you first started testing this branch. Try removing and reinstalling the library from the head of the development repo:

pip uninstall python-libxdo
pip install -e 'git+https://github.com/rshk/python-libxdo.git#egg=python-libxdo'

I'd like to hold off on merging this PR until there is a stable version of python-libxdo available on pypi. Right now the package is broken if you try installing the latest version from pypi.

See this issue: rshk/python-libxdo#16

[calmofthestorm]

button

[calmofthestorm]

I'm confused -- are you sure you ran integration_test.py with the version of the code in this PR? Without the change from direction->button, it fails for me in test_drag_mouse.

Once I make the fix all tests pass.

Once you've fixed that I'm happy to merge this once libxdo releases a stable version that fixes our issues. (If you'd like me to merge it sooner since it's in a non-default option I'd be willing to.)

[mzizzi]

You're right. I wasn't running the test suite correctly. Good catch on the direction button issue (again).

Would you mind giving 672f0fb a go? I still run into issues with tests for move_mouse with reference=relative_active.

[mzizzi]

As of 646f1c2 requirements.txt points to working versions of python-libxdo and python-xlib. The last piece of the puzzle is making the integration tests pass. After that this should be good to merge.

[calmofthestorm]

To make things more confusing, reference=relative_active works fine for me with your changes (or at least that part of the unit test does).

Could you elaborate on the issues you were seeing?

[mzizzi]

I'm failing the assertion as described below.

$ python integration_test.py
Start the server, maximize the test window on your primary monitor, 
put the mouse in the center, select the window, and then hit space 
to begin the automated test.

Beginning test.
Testing test_get_context
Testing test_key_press
Testing test_write_text
Testing test_click_mouse
Testing test_move_mouse
Traceback (most recent call last):
  File "integration_test.py", line 323, in <module>
    test(test_move_mouse, communication.server, window)
  File "integration_test.py", line 297, in test
    method(rpc, window)
  File "integration_test.py", line 223, in test_move_mouse
    assert event.event_y == 75
AssertionError

[calmofthestorm]

weird. I just tried a number of various arrangements and they all work for me. Can you tell me more about your setup -- number of monitors, resolution, window manager/desktop environment, and how you had the windows laid out for the test?

Does reference=relative_active work for you when you test it manually (e.g., fire up a server and then send RPCs to it from python)?

Does the unit test pass for you with the xdotool backend? Or fail for both?

[mzizzi]

The issue appears to have been the cinnamon desktop. I recently switched to an Ubuntu 16.04 machine running the unity desktop and the integration tests work just fine.

[mzizzi]

@calmofthestorm thoughts?

[calmofthestorm]

Uh I've lost state on this. Were there any other reasons we need to wait to merge this?

Regarding desktop environments: In general I consider their issues out of scope for Aenea. Modern DEs are doing all sorts of crazy stuff I don't really want to put in the effort to support, and as far as I'm concerned once we have a way to ship events out of the VM to the x server our responsibility is ended. (Whenever something other than X finally starts getting real traction I'm open to supporting it, but that's another story).

[mzizzi]

Uh I've lost state on this. Were there any other reasons we need to wait to merge this?

I believe that I put the brake on merging this one in at first. I was having trouble getting the integration test suite to pass successfully on my machine running the Cinnamon desktop.

Regarding desktop environments: In general I consider their issues out of scope for Aenea. Modern DEs are doing all sorts of crazy stuff I don't really want to put in the effort to support, and as far as I'm concerned once we have a way to ship events out of the VM to the x server our responsibility is ended. (Whenever something other than X finally starts getting real traction I'm open to supporting it, but that's another story).

I agree 100%.

[mzizzi]

@calmofthestorm How do you feel about this one? I'd like to enable using python libxdo bindings as an input option.

[mzizzi]

@calmofthestorm Just to clarify, this won't break or change much of the existing xdotool server implementation. The default behavior for launching the aenea linux server will remain unchanged as well. All we're doing here is adding an option to launch the server with speedier input emulation.

[calmofthestorm]

Thanks again for doing this, and sorry for the high latency.