This PR speeds up the daemonization of a process, by introducing a faster way to create the ranges of the file descriptors to be closed. This is especially necessary in environments with a high limit of open file descriptors, e.g., Docker containers, where the highest fd can be 1048576.

Thank you for writing test scenarios that cover the code you've written.

This ‘_get_candidate_file_descriptor_ranges_TestCase’ scenario (‘exclude-all’) exposes a bug in ‘_get_candidate_file_descriptor_ranges’ (I have reported issue #39 for this).

I will add the test scenarios you've created (and issue #39 needs to be corrected).

The changes you've introduced are rather greater than needed to fix that bug. They don't seem to be needed to fix the behaviour tested by those test cases.

Is there some way to demonstrate, in an automated test, what the problem is? That way I can reproduce it (maybe there's an existing issue here?) and verify that the changes indeed fix the problem.

Why is this logic not in the existing ‘_get_candidate_file_descriptors’ function?

How is this logic helping where the ‘_get_candidate_file_descriptors’ did not?

Can you write a test that will fail with the existing ‘_get_candidate_file_descriptors’, to show what problem is being addressed here?

Hi Ben, thanks for taking the time to look into my PR.

The reason you see more changes than what's strictly necessary for #39 is because the main goal of this PR is not to fix this corner case, but to make the daemonization faster. Fixing it was more of a side-effect.

So, to demonstrate the slow daemonization time of python-daemon, when the maximum number of open files is 1048576, I've written a benchmark that you can see in this Github gist. There, you can also find the results of a benchmark run in my machine, which show a 4x-6x performance gain, depending on the Python version that one uses.

I hope that this clarifies the scope of this PR, and shows why we need a faster way to calculate the file descriptor ranges. If you're OK with the changes, I can rebase the PR over the current master branch.

In this line, we don't create the candidate file descriptors list, but the excluded file descriptors list. This is the opposite of what _get_candidate_file_descriptors did, which is why it was removed, along with its tests. This is also the main argument of the PR, that calculating the ranges of file descriptors based on the excluded fds, rather than all the possible ones, is much faster.

Note that the linked Python script goes to great lengths to ensure that the benchmark will run in an environment with the proper limits and python-daemon versions. If you have all that, you can skip the script and simply check in a Python interpreter how long the following takes, for the two different python-daemon versions:

daemon._get_candidate_file_descriptor_ranges(set())

Thanks for responding to explain further.

I am interested in the change you're describing, but I'd like to see the changes a little more focused on just that difference. As it is, the merge request obscures the intention of the change a bit too much.

If you still have the interest to do so, can I ask that you:

• Re-base onto the current ‘master’ (which adds test scenarios equivalent to those here). or if you choose, you can make a new merge request starting from current ‘master’.
• Implement the different algorithm you describe, in a function equivalent to the existing ‘_get_candidate_file_descriptors’. The name you choose for your new function will give a better idea of your concept for how this will be different.
• Ideally, with test cases and scenarios for your new function (equivalent to those today for ‘_get_candidate_file_descriptors’).
• Call that new function from ‘_get_candidate_file_descriptor_ranges’ with minimal alteration, to make clear the core difference from current behaviour.

I've rebased the PR onto the current master, and made the following changes:

1. Added a helper function for skipping invalid file descriptors, i.e., out of bounds values, and added some tests as well.
2. Fixed a corner-case in my PR where, if an excluded file descriptor was out of bounds, the candidate file descriptor range would include invalid file descriptors. I also added some tests to ensure that never happens.
3. Added some more comments in the algorithm and slightly altered it, to make it more readable.

As per your suggestion, I tried to break the algorithm in two parts, a helper function like _get_candidate_file_descriptors and minimum changes in _get_candidate_file_descriptor_ranges, but I'm afraid it's not that simple. The whole algorithm is a simple for-loop over the excluded file descriptors and a check at the end. Breaking it in two parts to fit the current way of doing things seems to make it less readable and concise.

Since the proposed implementation is the half of the previous one, could you perhaps consider it as a drop-in replacement and review it as such? I'd be more than eager to improve the code or the comments, if you find them lacking.

Experimenting with these changes, it seems this omission (the call to ‘get_maximum_file_descriptors’) accounts for a large difference in speed between the two. A lot of the rest is explained by never creating the ‘range(0, maxfd)’ as a result.

So that raises a problem: this implementation entirely ignores the ‘get_maximum_file_descriptors’ function, and thereby causes the library to no longer respond to changes in the ‘resource.getrlimit(resource.RLIMIT_NOFILE)’ value.

When I modify this function to get ‘maxfd’ via the ‘get_maximum_file_descriptors’ call, the performance improvement almost disappears.

I'm open to performance improvements, but not at the expense of losing correct behaviour given OS-level resource limits.

Can you suggest a way to keep using ‘get_maximum_file_descriptors’ while achieving a speed improvement?

Many of these new scenarios were recently incorporated (you can see some of these duplicated earlier in the existing collection of scenarios for this test case class).

I have de-duplicated these additional scenarios and merged them to ‘master’ now (as of (commit e3fe6753)[https://pagure.io/python-daemon/c/e3fe675347d1d2463e43254134a961ea1ef353e0?branch=master]).

My bad, they must have slipped while rebasing over the new master. Thanks for sorting this out.

Maybe I'm missing something here but this function does use the maximum file descriptor value. It accepts it as an argument, and the caller calculates it using the get_maximum_file_descriptors() function. This happens in the _get_candidate_file_descriptor_ranges() function (a few lines below), which uses the maximum number of file descriptors as well.

Hi Ben, just checking, do you have any other questions regarding this PR? Have I covered your previous question? This improvement is kind of important for us (Arrikto), so we'd really like to see it as part of python-daemon.

Also, out of curiosity, you said that you experimented with the changes. Did you happen to run the benchmark script that I mentioned here, and if so, did you see the same speed-up?

On Thu, 2019-06-27 01:35 +10:00, Alex Pyrgiotis pagure@pagure.io wrote:

Hi Ben, just checking, do you have any other questions regarding this
PR? Have I covered your previous question? This improvement is kind of
important for us (Arrikto), so we'd really like to see it as part of
python-daemon.

Thanks for keeping on top of this merge request.

Is there an existing Pagure issue here that describes the problem being solved by this request? If not, can you describe the problem in a new issue?

--
\
`\
_o__) Ben Finney ben@benfinney.id.au

I've opened a new issue (#40) where I describe the problem in detail. Feel free to comment on that.

Regarding my previous question:

Also, out of curiosity, you said that you experimented with the changes. Did you happen to run the benchmark script that I mentioned here, and if so, did you see the same speed-up?

Could you please check out the benchmark script, to ensure that you see the same speed-up?

I don't like bumping threads, but I'm afraid that this one has stalled for a while :-/. Is there something hindering the review of this PR? I totally get it if it's at the bottom of your priority list, but if there's any concern that I can help clarify, do tell. I understand that we're dealing with a change in a critical path that is happening for performance reasons and not for correctness, but I think it's quite short and adequately tested.

At the end of the day, there's no point in leaving this PR in limbo, given that we've already benchmarked it and commented on the code.

I am looking for changes that still preserve the meaning of the _get_candidate_file_descriptor_ranges function. An alternative is the implementation in merge request 43.

I am looking for changes that still preserve the meaning of the _get_candidate_file_descriptor_ranges function. An alternative is the implementation in merge request 43.

The implementation has now been merged (at commit aac98d4f). I'm closing this merge request.

Feel free to make a new merge request with further improvements you would like.