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

gh-117709: Add vectorcall support for positional-only arguments of str() #117746

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Apr 11, 2024

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Apr 11, 2024

The speedups are as with #117725, but only for the positional-only cases. Summary of the benchmarks1:

  • str(): 2x faster (not that it matters much; it's not a very interesting use-case)
  • str(x): no change
  • str(x, enc): 2x faster
  • str(x, enc, err): 3x faster
  • fallback for any case with keyword args: no change

Footnotes

  1. release build on macOS.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I prefer this PR over #117725 which is way more complex.

Objects/unicodeobject.c Outdated Show resolved Hide resolved
Objects/unicodeobject.c Outdated Show resolved Hide resolved
Objects/unicodeobject.c Outdated Show resolved Hide resolved
Objects/unicodeobject.c Outdated Show resolved Hide resolved
Objects/unicodeobject.c Outdated Show resolved Hide resolved
}
return str;
}

Copy link
Member

Choose a reason for hiding this comment

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

You may add a comment to mention that this function is a fast-path for positional-only cases.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

I personally prefer this one :)
Do we have enough tests for positional arguments cases?

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Apr 11, 2024

I personally prefer this one :) Do we have enough tests for positional arguments cases?

Well, we could add something like this:

diff --git a/Lib/test/test_str.py b/Lib/test/test_str.py
index b4927113db..ea37eb5d96 100644
--- a/Lib/test/test_str.py
+++ b/Lib/test/test_str.py
@@ -2651,6 +2651,24 @@ def test_check_encoding_errors(self):
         proc = assert_python_failure('-X', 'dev', '-c', code)
         self.assertEqual(proc.rc, 10, proc)
 
+    def test_str_invalid_call(self):
+        check = lambda *a, **kw: self.assertRaises(TypeError, str, *a, **kw)
+
+        # too many args
+        check(1, "", "", 1)
+
+        # no such kw arg
+        check(test=1)
+
+        # 'encoding' must be str
+        check(1, encoding=1)
+        check(1, 1)
+
+        # 'errors' must be str
+        check(1, errors=1)
+        check(1, "", errors=1)
+        check(1, 1, 1)
+
 
 class StringModuleTest(unittest.TestCase):
     def test_formatter_parser(self):

UPDATE: added in 8138db4

Objects/unicodeobject.c Outdated Show resolved Hide resolved
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I added a suggestion.

I'm not sure that arg_as_utf8() helper is strictly needed, you can keep it or inline it, it's up to you ;-)

Objects/unicodeobject.c Outdated Show resolved Hide resolved
Co-authored-by: Victor Stinner <vstinner@python.org>
@erlend-aasland erlend-aasland enabled auto-merge (squash) April 11, 2024 13:30
@erlend-aasland
Copy link
Contributor Author

Thank you so much for the reviews, Donghee and Victor!

@erlend-aasland erlend-aasland merged commit 044dc49 into python:main Apr 11, 2024
36 checks passed
@erlend-aasland erlend-aasland deleted the perf/unicode-vectorcall-pos-only branch April 11, 2024 13:55
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…y arguments (python#117746)

Fall back to tp_call() for cases when arguments are passed by name.

Co-authored-by: Donghee Na <donghee.na@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
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.

None yet

3 participants