python / cpython

The Python programming language
https://www.python.org
Other
63.03k stars 30.19k forks source link

Clean up division fast paths in Objects/longobject.c #72247

Open mdickinson opened 8 years ago

mdickinson commented 8 years ago
BPO 28060
Nosy @mdickinson, @pitrou, @serhiy-storchaka
Files
  • divmod_fastpath.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['performance'] title = 'Clean up division fast paths in Objects/longobject.c' updated_at = user = 'https://github.com/mdickinson' ``` bugs.python.org fields: ```python activity = actor = 'mark.dickinson' assignee = 'none' closed = False closed_date = None closer = None components = [] creation = creator = 'mark.dickinson' dependencies = [] files = ['44528'] hgrepos = [] issue_num = 28060 keywords = ['patch'] message_count = 6.0 messages = ['275618', '275619', '275675', '275679', '298843', '298845'] nosy_count = 3.0 nosy_names = ['mark.dickinson', 'pitrou', 'serhiy.storchaka'] pr_nums = [] priority = 'normal' resolution = None stage = 'commit review' status = 'open' superseder = None type = 'performance' url = 'https://bugs.python.org/issue28060' versions = ['Python 3.6'] ```

    mdickinson commented 8 years ago

    We seem to have ended up with redundant fast path checks for division in longobject.c: long_div has a fast path check, but the long_div slow path calls l_divmod, which then does a second, identical, fast path check. long_mod has similar behaviour. long_divmod, however, has no fast path, so relies on the one from l_divmod.

    This patch removes the extra fast path from l_divmod, and then adds a top-level fast path check to long_divmod.

    mdickinson commented 8 years ago

    N.B. The patch also tweaks the fast path condition to *include the common case of a dividend of 0, and *exclude the rare case of a negative divisor. (The latter change helps to keep the fast path code simple.)

    mdickinson commented 8 years ago

    Serhiy: thanks for the review. Replying here, since I get a 500 error every time I try to reply from Rietveld.

    I'd rather not rely on either NSMALLPOSINTS > 0 or on digit 0 existing when Py_SIZE is 0. We don't rely on that elsewhere, and the code should stay simple where possible.

    serhiy-storchaka commented 8 years ago

    The patch LGTM. I'll open separate issue for guaranteeing that a->ob_digit[0] is 0 in case of Py_SIZE(a) == 0 and using this fact for simplifying and optimizing the code.

    pitrou commented 7 years ago

    What's the status of this?

    mdickinson commented 7 years ago

    What's the status of this?

    Forgotten, rather than abandoned. :-( I'll unassign so that someone else can pick this up. I believe the patch is ready to go, except that of course now it needs a PR.