Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

bpo-38938: Adjusting the heapq.merge algorithm for fewer comparisons #17729

Open
wants to merge 9 commits into
base: master
from

Conversation

@sweeneyde
Copy link
Contributor

sweeneyde commented Dec 28, 2019

This pull request would:

  • Change heapq.merge to be a class whose instances are iterable objects, rather than a generator function.
  • Change the algorithm for heapq.merge:
    • Before this PR, we repeatedly applied heapreplace on (index, iterator, item) tuples
      • Starting at the root, this would repeatedly replace parents with their smaller child. Upon reaching a leaf, it would replace that leaf using the __next__ method of the iterator corresponding to the the item we are replacing. It would then sift that new leaf down into the appropriate part of the heap.
    • After this PR, we would apply a "tree sift" method:
      • General algorithm: the items compete in a tournament, as we maintain a binary "tree of losers" (see Knuth Volume 3, Chapter 5.4.1. on "Multiway Merging").
      • Here, we only need to store individual items, or key-item pairs.
      • After popping off the root, we replace it with its smaller child, the child with its smaller child, etc., until reaching a leaf. So far, this is identical to the standard siftup method.
      • However, once reaching a leaf, we replace the leaf with the iterator corresponding to the index of that leaf. That is, always filling the leftmost leaf with the first iterable that was passed, the second-leftmost with the second-passed, ..., and the rightmost leaf with the last iterable passed.
      • This eliminates the need to sift the leaf down afterward because we already know that it is larger than the item it replaces.
      • For tree sizes that aren't a power of two, permuting the iterators so that the first one is leftmost is simply a matter of shifting all iterators around the list by a few spots.
      • Heuristically, removing the requirement that there be exactly one item from each iterator in the heap at a time makes the heap more likely to consist of those items which are closest.
      • Storing the tree flatly (for the C implementation, even when using a key), rather than having a tuple at each node helps with cache locality.
      • This is a non-recursive version of recursively applying a function that merges two iterables.
  • Add the corresponding C and Python implementations.
  • Add a test case for error handling in heapq.merge

According to my benchmarking, this is at least 2.5 times as fast in almost all cases to the existing implementation, and is especially fast (about 5x as fast as the existing way) with small numbers of iterables. I tested up to 12,000 iterables, with varying degrees of "balance" among the number of items in each. Also noteworthy is that fewer comparisons on average were needed with the proposed new method.

https://bugs.python.org/issue38938

Copy link
Member

pablogsal left a comment

I made a quick review of general aspects of the PR and left some comments. When I have more time I can do a more in-depth review of the PR.

Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Show resolved Hide resolved
Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Outdated Show resolved Hide resolved
Modules/_heapqmodule.c Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Dec 29, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

merge_dealloc(mergeobject *mo)
{
PyObject_GC_UnTrack(mo);
Py_XDECREF(mo->tree);

This comment has been minimized.

Copy link
@pablogsal

pablogsal Dec 30, 2019

Member

You can reuse merge_clear here to clear the references.

@sweeneyde

This comment has been minimized.

Copy link
Contributor Author

sweeneyde commented Dec 30, 2019

I have made the requested changes; please review again.

What would be best type for mergeobject.tree and mergeobject.iterators? I've come up with 3 ideas:

  1. Leave them as lists, opening up the possibility of unexpected modification if someone uses gc.get_objects(). I think the line arr[childpos] = Py_None could cause refleaks in this case, and so it would have to change to swapping arr[pos] and arr[childpos].
  2. Make them tuples, preventing their modification from Python code, but violating the immutability of tuples.
  3. Use a PyObject ** raw c-array, preventing any outside access, but potentially making garbage collection, maintenance, and debugging messier.

Or is there some better option?

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Dec 30, 2019

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from pablogsal Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.