python-daemon

#69 Refactor `_get_candidate_file_descriptor_ranges()` to iterate over excludes rather than all descriptors.
Closed by bignose. Opened by ikholopov.
ikholopov/python-daemon remove-in-memory-fd-list  into  main

Download 69.patch

The problem with _get_candidate_file_descriptor_ranges() is that in _total_file_descriptor_set a set of the size is initialized.

While it works well for smaller limits, like 2048, the limit that I've encountered was 1073741816. This resulted in a simple import statement import daemon triggering the attempt to create a full set of numbers of that size, which quickly ate all of 64GBs of memory that I had and crashed :)

Based on the only usage of _total_file_descriptor_set which is in a helper _get_candidate_file_descriptor_ranges function, I don't think it is actually needed. We operate with ranges, and not need an actual list of descriptors to be stored in memory. In my first attempt to tackle this, I just replaced the logic of iteration to use a calculated range instead of a set object. This helped with memory footprint, but there still was a performance bottleneck - the time to iterate over 1B items in Python is still not fast (for ex. the newly test would took hours).

So, instead of iterating over the candidate descriptors, the _get_candidate_file_descriptor_ranges got rewritten to iterate over exclusions. Contrary to the total range, for this one we can assume that it will be relatively small (almost certainly less that 1000). The logic of building ranges didn't change drastically either - we build ranges, but instead of keeping track and updating the last added range, the not-yet-processed part is being tracked and updated after each excluded descriptor that can split it into two ranges is processed.

A number of helper functions are cleaned up as they are no longer needed.

I like where this is going, thank you for beginning this.

Thank you for making a test condition for invalid exclude values.

Instead of silently ignoring those values, I have instead implemented a unit test to ensure those values will raise a TypeError. You can re-base from 'main' branch which now includes commit 46372dc7, and get that new unit test to assert the behaviour.

The full test suite (and build system) is currently failing because of issue #70, so I will give attention to that first.
Edited by bignose

rebased onto 578f7dac2c32e609b1bec106ca5d47c3c72ef6d0

Thank you for making a test condition for invalid exclude values.

Instead of silently ignoring those values, I have instead implemented a unit test to ensure those values will raise a TypeError. You can re-base from 'main' branch which now includes commit 46372dc7, and get that new unit test to assert the behaviour.

Great! I've rebased and updated the code to make use of the new validation.

The full test suite (and build system) is currently failing because of issue #70, so I will give attention to that first.

That issue is now closed, so the test suite should pass the 'main' branch again in Python 3.10 and above.

Can you rebase onto 'main' branch again, try the test suite and confirm?
Edited by bignose

I have extracted a set of changes (current in 'main' as of commit c72a3d7f) in order to focus the merge request changes to those crucial to your refactoring.

Using a timing test suggested by @apyrgio, I have demonstrated the refactoring greatly speeds up the computation for closing open files.

With the existing resource-intensive computation:

$ bash ../timeit_candidate_fds.sh
daemon.daemon.get_maximum_file_descriptors() returns 104857600
[…]
Timings for ‘daemon.daemon._get_candidate_file_descriptor_ranges(set())’
1 loop, best of 5: 9.47 sec per loop

With the refactored implementation informed by this merge request:

$ bash ../timeit_candidate_fds.sh
daemon.daemon.get_maximum_file_descriptors() returns 104857600
[…]
Timings for ‘daemon.daemon._get_candidate_file_descriptor_ranges(set())’
500000 loops, best of 5: 636 nsec per loop

That's a very big improvement! No surprise, because now we are avoiding the allocation of a concrete set of file descriptors to represent the entire range, and just using range objects with the os.closerange function.
Edited by bignose

Given the improvement demonstrated, I have merged these changes (as edited on top of the extracted changes, to focus on the refactor). The 'main' branch now has these changes as of commit 297e91ab.

Thank you for the work to bring this improvement.

Pull-Request has been closed by bignose

Thanks!
Sorry for not getting back to it earlier, great to see it merged!
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.