#14 poor performance in _get_candidate_file_descriptor_ranges
Closed: Insufficient data by bignose. Opened by pandorasfox.

Essentially, even with the os.closerange improvement, _get_candidate_file_descriptor_ranges still tended to perform relatively poorly (in fact, it took my raspberry pi ~30s with this method, compared to ~20s with the former "try every fd" method).

Upon some investigation, it ended up being mostly due to this loop:

for fd in candidates_list[1:]:

Since it'd end up iterating for quite a while to build the last range.

Ideally, since we should know maximum excluded fd and the maximum possible fd, it should be fairly straightforward to just check if we're over that last fd and just generate the last range without iteration.

Alternatively, we can poke /proc/ to get a list of our open FDs, then just use that to have a much smaller range.

PR #13 addresses this.


Thank you for the investigation, and for thinking of different ways to address this issue.

Ideally, since we should know maximum excluded fd and the maximum possible fd, it should be fairly straightforward to just check if we're over that last fd and just generate the last range without iteration.

I'd like to see a set of unit test scenarios that exercise different edge cases, to understand what behaviour you are expecting in different scenarios.

Alternatively, we can poke /proc/ to get a list of our open FDs, then just use that to have a much smaller range.

Does that work across all Unix operating systems supported by Python?

Metadata Update from @bignose:
- Issue tagged with: moreinfo

_get_candidate_file_descriptor_ranges is VERY inefficient in building the file descriptor ranges. For example on my Raspberry Pi-like board that defaults to RLIMIT_NOFILE=1048576, it takes 13.7 seconds to run, even before any attempt is made to close those file descriptors (!)

Instead of building a potentially huge array of 1 million numbers, and then iterating through that, I suggest iterating through the significantly smaller list of excluded file descriptors:

# _get_candidate_file_descriptors() is no longer used
def _get_candidate_file_descriptor_ranges(exclude):
    """ Get the collection of candidate file descriptor ranges.
        :param exclude: A collection of file descriptors that should
            be excluded from the return ranges.
        :return: The collection (a `list`) of ranges that contain the
            file descriptors that are candidates for files that may be
            open in this process.
        Determine the ranges of all the candidate file descriptors.
        Each range is a pair of `int` values (`low`, `high`).
        A value is a candidate if it could be an open file descriptor
        in this process, excluding those integers in the `exclude`
        collection.
        The `maxfd` value is determined from the standard library
        `resource` module.
        """
    minfd = 0
    maxfd = get_maximum_file_descriptors()
    exclude = sorted(exclude)
    ranges = []
    for fd in exclude:
        if minfd != fd:
            ranges.append(FileDescriptorRange(low=minfd, high=fd))
        minfd = fd + 1
    if minfd < maxfd:
        ranges.append(FileDescriptorRange(low=minfd, high=maxfd))
    return ranges

this now takes 44 microseconds to run with an empty 'exclude', some 311,000x faster. Or 74 microseconds with a modest list of file descriptors to exclude, 185,000x faster.

>>> exclude=[10,11,5,200,10000]
>>> daemon._get_candidate_file_descriptor_ranges(exclude)
[FileDescriptorRange(low=0, high=5), FileDescriptorRange(low=6, high=10), FileDescriptorRange(low=12, high=200), FileDescriptorRange(low=201, high=10000), FileDescriptorRange(low=10001, high=1048576)]

@sheffieldnick, thanks for the suggestion. Can you make this into a merge request, with test coverage? That will allow people on non-Linux systems to check whether this helps there.

lol. You've got all my code, so feel free to cut'n'paste that into your library, and write any tests you feel it needs.

And you can see just from looking at the new algorithm that it makes no OS-specific calls, so the performance gain will be on all systems.

lol. You've got all my code

Thanks for the suggestion. Without a merge request or tests, I must be honest that this won't receive much attention.

Metadata Update from @bignose:
- Issue close_status updated to: Insufficient data
- Issue status updated to: Closed (was: Open)

Metadata