GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-90043: Handle NaNs in `COMPARE_OP_FLOAT_JUMP` by brandtbucher · Pull Request #100278 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Dec 15, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

COMPARE_OP_FLOAT_JUMP contains two DEOPT_IF branches for NaN values. This means that COMPARE_OP will repeatedly specialize and deoptimize in the presence of NaNs.

The current scheme can be slightly modified to handle unordered operands correctly, and remove these checks entirely.

  • Issue: Specialize COMPARE_OP #90043

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
brandtbucher added 5 commits November 28, 2022 08:24
@brandtbucher
Handle NaNs in COMPARE_OP_FLOAT_JUMP
f0728cf
@brandtbucher
Remove NaN checks!
2665c9a
@brandtbucher
Catch up with main
dba5894
@brandtbucher
Catch up with main
66d2d63
@brandtbucher
blurb add
9dc68aa
@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Dec 15, 2022
@brandtbucher
brandtbucher requested review from markshannon and sweeneyde December 15, 2022 21:05
@brandtbucher brandtbucher self-assigned this Dec 15, 2022
@bedevere-bot bedevere-bot mentioned this pull request Dec 15, 2022
Specialize COMPARE_OP #90043
Closed
@netlify

netlify Bot commented Dec 15, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview ready!

Name Link
🔨 Latest commit 9dc68aa
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639b8c16d9b4720008a2e72e
😎 Deploy Preview https://deploy-preview-100278--python-cpython-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting core review label Dec 15, 2022
sweeneyde
sweeneyde approved these changes Dec 16, 2022
View reviewed changes
Comment thread Python/bytecodes.c
DEOPT_IF(isnan(dright), COMPARE_OP);
STAT_INC(COMPARE_OP, hit);
// 1 if NaN, 2 if <, 4 if >, 8 if ==; this matches when_to_jump_mask
int sign_ish = 1 << (2 * (dleft >= dright) + (dleft <= dright));

@sweeneyde sweeneyde Dec 15, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever -- roughly the same number of instructions too!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Dec 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Credit to @markshannon for figuring this out. I had tried something similar (with >= and <=), but couldn't get the masks to work correctly without his help.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson Dec 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially a bit dangerous: a comparison with NaN may raise FE_INVALID. It may be safer to use the isless macro instead.

From C99 (§7.12.14.3):

The isless macro determines whether its first argument is less than its second argument. The value of isless(x, y) is always equal to (x) < (y); however, unlike (x) < (y), isless(x, y) does not raise the ‘‘invalid’’ floating-point exception when x and y are unordered.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that should be islessequal of course, not isless.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@markshannon markshannon Dec 16, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float_richcompare uses simple i < j comparisons. Why is that OK, but not here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandtbucher No, not specific to signalling NaNs, just to comparisons involving NaNs. And yes, float_richcompare should be updated to use the safer code, now that we're allowed to depend on C99 features.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Dec 16, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll merge this, and create a separate issue where we can discuss how/if we should avoid potential floating-point exceptions everywhere (since that seems like a much bigger change).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdickinson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@mdickinson mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, it's platform-specific. I think we're in a place now where all current platforms do the "right" thing, but that wasn't always the case. There were a few related issues in the past - #81655 is the only one I'm finding right now, though. So yes, probably a non-issue at this point.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a separate issue

Thanks. Don't worry about this, though - I think I'm fussing about things that were historically an issue, but are no longer. It's not that long ago that we did protect all floating-point operations, e.g., here's float_richcompare in Python 2.7:

cpython/Objects/floatobject.c

Lines 600 to 621 in ca079a3

PyFPE_START_PROTECT("richcompare", return NULL)
switch (op) {
case Py_EQ:
r = i == j;
break;
case Py_NE:
r = i != j;
break;
case Py_LE:
r = i <= j;
break;
case Py_GE:
r = i >= j;
break;
case Py_LT:
r = i < j;
break;
case Py_GT:
r = i > j;
break;
}
PyFPE_END_PROTECT(r)

But it seems that all current platforms do follow the IEEE 754 standard's advice on "default" floating-point exception handling settings, so this is something we no longer need to worry about.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@mdickinson mdickinson Dec 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(FTR, it looks like Python 3.9 is where we discarded the PyFPE_START_PROTECT / PyFPE_END_PROTECT guards everywhere. They're still present in 3.8.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/specialize.c
int when_to_jump_mask = compare_masks[oparg];
if (next_opcode == POP_JUMP_IF_FALSE) {
when_to_jump_mask = (1 | 2 | 4) & ~when_to_jump_mask;
when_to_jump_mask = (1 | 2 | 4 | 8) & ~when_to_jump_mask;

@sweeneyde sweeneyde Dec 15, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was skeptical of this at first, since I had thought about this as "use _Py_SwappedOp[op]", which would be wrong. But this looks correct -- it only swaps which branch to take.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Dec 16, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. The swapped ops used to be each others' bitwise inversions. That's no longer the case for the "unordered" bit, since NaNs only care if the operation is != or not.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sweeneyde reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Dec 16, 2022
markshannon
markshannon approved these changes Dec 16, 2022
View reviewed changes
@brandtbucher
brandtbucher merged commit 9076455 into python:main Dec 16, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 16, 2022
carljm added a commit to carljm/cpython that referenced this pull request Dec 16, 2022
@carljm
Merge branch 'main' into fastmock
1b6241a
* main:
  pythongh-99540: Constant hash for _PyNone_Type to aid reproducibility (pythonGH-99541)
  pythongh-100039: enhance __signature__ to work with str and callables (pythonGH-100168)
  pythongh-99830: asyncio: Document returns of remove_{reader,writer} (python#100302)
  "Compound statement" docs: Fix with-statement step indexing (python#100286)
  pythonGH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP (pythonGH-100278)
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Dec 18, 2022
@shihai1991
Merge remote-tracking branch 'origin/main' into bpo_34697
2878571
* origin/main: (1306 commits)
  Correct CVE-2020-10735 documentation (python#100306)
  pythongh-100272: Fix JSON serialization of OrderedDict (pythonGH-100273)
  pythongh-93649: Split tracemalloc tests from _testcapimodule.c (python#99551)
  Docs: Use `PY_VERSION_HEX` for version comparison (python#100179)
  pythongh-97909: Fix markup for `PyMethodDef` members (python#100089)
  pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890)
  pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890)
  pythonGH-98831: Add DECREF_INPUTS(), expanding to DECREF() each stack input (python#100205)
  pythongh-78707: deprecate passing >1 argument to `PurePath.[is_]relative_to()` (pythonGH-94469)
  pythongh-99540: Constant hash for _PyNone_Type to aid reproducibility (pythonGH-99541)
  pythongh-100039: enhance __signature__ to work with str and callables (pythonGH-100168)
  pythongh-99830: asyncio: Document returns of remove_{reader,writer} (python#100302)
  "Compound statement" docs: Fix with-statement step indexing (python#100286)
  pythonGH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP (pythonGH-100278)
  Improve stats presentation for calls. (pythonGH-100274)
  Better stats for `LOAD_ATTR` and `STORE_ATTR` (pythonGH-100295)
  pythongh-81057: Move the Cached Parser Dummy Name to _PyRuntimeState (python#100277)
  Document that zipfile's pwd parameter is a `bytes` object (python#100209)
  pythongh-99767: mark `PyTypeObject.tp_watched` as internal use only in table (python#100271)
  Fix typo in introduction.rst (python#100266)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes
@sweeneyde sweeneyde sweeneyde approved these changes
+1 more reviewer
@mdickinson mdickinson mdickinson left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@brandtbucher @mdickinson @markshannon @sweeneyde @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.