Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Robustify Process handling and extend a bit #127

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Robustify Process handling and extend a bit #127

wants to merge 9 commits into from

Conversation

krono
Copy link
Contributor

@krono krono commented Oct 1, 2019 •
edited
Loading

  • Isolate persistence writing to a single thread
    (trivial in default exclusive case, not possible in non-exclusive case)
  • Add a shaed-queue-based scheduler for parallel execution
  • Caveat lector: Adds a new dependency: subprocess32

krono added 5 commits October 1, 2019 16:24
@krono
Robustify subprocess handling
ed23f59 
Python from 3.2 on support timeouts on subprocesses directly
subprocess32 backports that to Python 2
@krono
Code defaults via default parameters.
5ac081c 
Easier to read. Also, `timeout=None` because that's what modern
subprocess expects
@krono
utilize humanfriendly where it helps
d46f79c
@krono
Robustify process handling in presence of threads
5ee64a0 
There are many caveats, but at least Ctrl-C now works as expected.
@krono
Isolate persistence writing to file to one thread
44fe332 
There were out-of-order writes to the persitence file when there were
multiple threads. This confines file-writing to a dedicated thread that
works a Queue.
@smarr
Copy link
Owner

smarr commented Oct 1, 2019

Sorry, I don't think I will get to this or the other PR in a while.
Here you seem to be changing one of the most crucial bits, and I want to be sure that I understand the consequences.

So, just briefly:
"Robustify subprocess handling" is unfortunately not helping me in understanding what was broken with the current solution.

I suppose, I am getting a bit defensive here. But so far, the current solution seem to have worked. While the new one is, well, new to me.

@krono
Copy link
Contributor Author

krono commented Oct 1, 2019

What was broken?

  • Ctrl-C did kill neither rebench nor the executable(s) in certain circumstances
  • Rebench wrote results out-of-order to the data file, which meant they did not read correctly on re-run and were hard to analyze.

This fixes both.

@krono
Copy link
Contributor Author

krono commented Oct 1, 2019 •
edited
Loading

Also, there were differences for Python2 and Python3 regarding process handling.

With subprocess32 this is more unified and does not require the custom Timeout handling anymore.

krono added 3 commits October 3, 2019 10:34
@krono
Revert changes to executor as to not interfere with set expectations
cc6b5f2
@krono
make sure we have a thread
f3012f1
@krono
Add queue-based pulling parallel scheduler
92e9701 
The existing parallel scheduler behaves as is
(With common behavior lifted to a new base clase).

A new parallel scheulder maintains a work queue and the workers retrieve
directly from there.

a distinction is made for py2 vs py3, to make use of the built-in thread
pool executor when possible.

No new magic so far
@krono
Copy link
Contributor Author

krono commented Oct 7, 2019

I reverted the changes to the existing parallel scheduler and made a new one.
The new one has a Queue where worker threads pull from.
Apparently, Queue.join() does not suffer the same problem as Thread.join on Python 2, so the previous hack is not necessary.

@smarr smarr added this to the later milestone May 2, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
later
Development

Successfully merging this pull request may close these issues.
2 participants
@krono @smarr