bpo-41873: Add vectorcall for float() by sweeneyde · Pull Request #22432 · python/cpython · GitHub
Skip to content

bpo-41873: Add vectorcall for float() - #22432

Merged
corona10 merged 4 commits into
python:masterfrom
sweeneyde:float_vectorcall
Sep 29, 2020
Merged

corona10 merged 4 commits into
python:masterfrom
sweeneyde:float_vectorcall

Conversation

@sweeneyde

@sweeneyde sweeneyde commented Sep 28, 2020

Copy link
Copy Markdown
Member

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your patch will cause the crash

Type "help", "copyright", "credits" or "license" for more information.
>>> float()
[1]    44448 segmentation fault  ./python.exe

@bedevere-bot

Copy link
Copy Markdown

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add the test case that I pointed out -> the case is already existed.

Comment thread Objects/floatobject.c Outdated
return NULL;
}

return float_new_impl((PyTypeObject *)type, args[0]);

@corona10 corona10 Sep 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
return float_new_impl((PyTypeObject *)type, args[0]);
PyObject *x = NULL;
if (nargs > 0) {
x = args[0];
} else {
x = _PyLong_Zero;
}
return float_new_impl((PyTypeObject *)type, x);

@corona10
corona10 requested a review from vstinner September 28, 2020 09:11
@sweeneyde

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

@corona10 I didn't see any occurrences of float() in test_float.py, so I added a test. Where are you saying the test already exists?

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@corona10

Copy link
Copy Markdown
Member

I didn't see any occurrences of float() in test_float.py

https://github.com/python/cpython/pull/22432/checks?check_run_id=1175574849

There was already a crash on your PR on CI phase.
So I guess that there is already in, but it looks like good to add the case on test_float.py

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am lgtm about the changes,
but let's wait for other core devs opinions about adding vectorcall for float type.

@corona10

corona10 commented Sep 28, 2020

Copy link
Copy Markdown
Member

@markshannon

This PR is same case from #22427

Do you think that the vectorcall is good enough to apply for float() call also from the maintenance cost view?

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. IMHO removing 27.5 ns on 79 ns (1.5x faster) per float(obj) is worth it.

I let @corona10 merge the PR ;-)

@corona10
corona10 merged commit e8acc35 into python:master Sep 29, 2020
@bedevere-bot

Copy link
Copy Markdown

shihai1991 added a commit to shihai1991/cpython that referenced this pull request Sep 29, 2020
* origin/master: (113 commits)
  bpo-41773: Raise exception for non-finite weights in random.choices().  (pythonGH-22441)
  bpo-41873: Add vectorcall for float() (pythonGH-22432)
  bpo-41861: Convert _sqlite3 PrepareProtocolType to heap type (pythonGH-22428)
  bpo-41842: Add codecs.unregister() function (pythonGH-22360)
  bpo-41875: Use __builtin_unreachable when possible (pythonGH-22433)
  bpo-40105: ZipFile truncate in append mode with shorter comment (pythonGH-19337)
  bpo-41870: Use PEP 590 vectorcall to speed up bool()  (pythonGH-22427)
  [doc] Leverage the fact that the actual types can now be indexed for typing (pythonGH-22340)
  bpo-41861: Convert _sqlite3 cache and node static types to heap types (pythonGH-22417)
  bpo-41858: Clarify line in optparse doc (pythonGH-22407)
  Revert "Fix all Python Cookbook links (python#22205)" (pythonGH-22424)
  bpo-1635741: Port _bisect module to multi-phase init (pythonGH-22415)
  bpo-41428: Fix compiler warning in unionobject.c (pythonGH-22416)
  Fix logging error message (pythonGH-22410)
  bpo-39934: Account for control blocks in 'except' in compiler. (pythonGH-22395)
  bpo-41775: Make 'IDLE Shell' the shell title  (python#22399)
  bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388)
  bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387)
  bpo-41833: threading.Thread now uses the target name (pythonGH-22357)
  bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633)
  ...
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants