bpo-36030: Add _PyTuple_FromArray() function #11954

sir-sigurd · 2019-02-20T11:30:04Z

@vstinner this is somewhat alternative to #11927.

https://bugs.python.org/issue36030

vstinner

I prefer this PR other your 2 other PRs. But please make the API private, or even internal.

Include/tupleobject.h

Objects/tupleobject.c

vstinner · 2019-02-20T13:25:04Z

Objects/tupleobject.c

@@ -419,14 +431,32 @@ tupleitem(PyTupleObject *a, Py_ssize_t i)
    return a->ob_item[i];
 }

+static PyObject *
+tuple_from_array(PyObject *const *src, Py_ssize_t n, Py_ssize_t step)


I dislike step. I prefer to leave tuplesubscript() unchanged and avoid step complication here.

Any reason why this complexity better fits for tuplesubscript() rather than for tuple_from_array()?

I prefer to keep tuple_from_array() simple. The compiler should be able to emit more efficient code without step.

The whole purpose of the change is performance, no?

If I'm not mistaken compiler does its job and compile PyTuple_FromArray() efficiently:

444 PyObject *item = src[cur]; 0x00000000000c1128 <+200>: mov (%r12,%rax,8),%rdx 445 Py_INCREF(item); 446 dst[i] = item; 0x00000000000c1130 <+208>: mov %rdx,0x18(%rbx,%rax,8) 0x00000000000c1135 <+213>: add $0x1,%rax 0x00000000000c1139 <+217>: cmp %rax,%rbp 0x00000000000c113c <+220>: jne 0xc1128 <PyTuple_FromArray+200>

Well, others may have a different opinion, but I still don't think that adding a step argument is worth it for a single call site: tuplesubscript(). I would prefer a very straightforward tuple_from_array() without step argument: well, it becomes _PyTuple_FromArray() in this case. No need for a subfunction.

sir-sigurd · 2019-02-20T16:40:10Z

Objects/call.c

@@ -1276,45 +1276,19 @@ PyObject_CallFunctionObjArgs(PyObject *callable, ...)
 _Py_NO_INLINE PyObject *
 _PyStack_AsTuple(PyObject *const *stack, Py_ssize_t nargs)


@vstinner
Does it make sense to have _PyStack_AsTuple() and _PyStack_AsTupleSlice() after adding PyTuple_FromArray()? Might make sense to remove them or replace with macros.

You can remove _PyStack_AsTuple, but please do that in a second PR (once this one is merged, if it's merged ;-)).

I would remove both _PyStack_AsTuple and _PyStack_AsTupleSlice. The former is just an alias of _PyTuple_FromArray, and the latter is an alias with trivial arguments transformation, and is used only once.

vstinner

The current impementation is tuple_new_internal() in unsafe: that's a blocker issue for me, see my comment.

I'm just a little bit afraid by usage of tuple_new_internal(). I would free more comfortable if you can split this PR in two parts:

Modify this PR to only add _PyTuple_FromArray() and use _PyTuple_FromArray(): "simplify the code"
Once the first PR is merged, modify tupleobject.c to use tuple_new_internal(): your micro-optimization

I also proposed a 3rd PR to remove _PyStack_AsTuple() and replace it with _PyTuple_FromArray().

A simpler PR would be easy to review and it would be easier to take a decision.

Objects/tupleobject.c

vstinner · 2019-02-21T15:11:09Z

Objects/tupleobject.c

@@ -419,14 +431,32 @@ tupleitem(PyTupleObject *a, Py_ssize_t i)
    return a->ob_item[i];
 }

+static PyObject *
+tuple_from_array(PyObject *const *src, Py_ssize_t n, Py_ssize_t step)


Well, others may have a different opinion, but I still don't think that adding a step argument is worth it for a single call site: tuplesubscript(). I would prefer a very straightforward tuple_from_array() without step argument: well, it becomes _PyTuple_FromArray() in this case. No need for a subfunction.

Objects/tupleobject.c

bedevere-bot · 2019-02-21T15:28:01Z

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.

sir-sigurd · 2019-02-25T05:21:20Z

I have made the requested changes; please review again

@vstinner
I left tuple_from_array() with step parameter, because I want to know other opinions on it.

bedevere-bot · 2019-02-25T05:21:23Z

Thanks for making the requested changes!

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

vstinner · 2019-02-25T08:10:22Z

@serhiy-storchaka, @methane, @rhettinger: Would you mind to review this change? IMHO the change is good, but without "tuple_from_array(PyObject *const *src, Py_ssize_t n, Py_ssize_t step)": without the step argument.

I asked @sir-sigurd to revert his optimization to not initialize items to NULL: he reverted it to make the PR simpler. He will propose a separated PR for this micro-optimization, once this one is merged.

IMHO this change is good because the API _PyTuple_FromArray() is easy to use, it removes a lot of code, and may open the way to further optimization later (that should be discussed later). Ah, and the new function is private ("internal"), so it can be reverted anytime if needed.

So the last thing is to remove the step argument. IMHO it's not worth it, it was used once in tuplesubscript. Keeping the existing tuplesubscript() code is better to avoid to make _PyTuple_FromArray() more complex and maybe slower (it's hard to check if the machine code is the most efficient on all architectures). But I expected than hardcoded step=1 helps the compiler and the CPU to run faster.

serhiy-storchaka

+1 if remove _PyStack_AsTuple and _PyStack_AsTupleSlice.

serhiy-storchaka · 2019-02-25T15:04:50Z

Objects/call.c

@@ -1276,45 +1276,19 @@ PyObject_CallFunctionObjArgs(PyObject *callable, ...)
 _Py_NO_INLINE PyObject *
 _PyStack_AsTuple(PyObject *const *stack, Py_ssize_t nargs)


I would remove both _PyStack_AsTuple and _PyStack_AsTupleSlice. The former is just an alias of _PyTuple_FromArray, and the latter is an alias with trivial arguments transformation, and is used only once.

vstinner · 2019-02-25T15:53:46Z

+1 if remove _PyStack_AsTuple and _PyStack_AsTupleSlice.

Yeah, I agree with that, but I asked @sir-sigurd to do that in a separated PR to keep this PR short and easier to review (see the length of the discussion of this PR ;-)).

methane · 2019-02-25T16:04:37Z

It removes 3x lines code. I agree this has net win. +1.

vstinner · 2019-02-25T16:08:21Z

@sir-sigurd: so please remove the "step" parameter.

vstinner

LGTM.

vstinner · 2019-02-25T17:00:55Z

Thanks @sir-sigurd, I merged your PR!

You can now work on a PR to remove _PyStack_AsTuple() (replace it with _PyTuple_FromArray) and maybe also remove _PyStack_AsTupleSlice. @serhiy-storchaka proposed to remove both.

I also would like a PR with your micro-optimization, it should now be way smaller, so easier to review.

erlend-aasland · 2021-05-20T17:59:18Z

Include/internal/pycore_tupleobject.h

@@ -11,6 +11,7 @@ extern "C" {
 #include "tupleobject.h"

 #define _PyTuple_ITEMS(op) (_PyTuple_CAST(op)->ob_item)
+PyAPI_FUNC(PyObject *) _PyTuple_FromArray(PyObject *const *, Py_ssize_t);


@vstinner Was there a reason to export this as an API function, iso. using the extern keyword?

I only started to no longer export symbols by using "extern" recently. Previously, I kept the habit of exporting symbols using PyAPI_FUNC/PyAPI_DATA. You can propose a PR to use extern.

I see. Would that affect the Stable API, @encukou?

No, nothing in Include/internal/ is in the limited API.

Great, thanks Petr.

FYI, see GH-26352 and bpo-44231.

sir-sigurd requested a review from rhettinger as a code owner February 20, 2019 11:30

the-knights-who-say-ni added the CLA signed label Feb 20, 2019

bedevere-bot added the awaiting review label Feb 20, 2019

sir-sigurd changed the title ~~36030: Add PyTuple_FromArray() function~~ bpo-36030: Add PyTuple_FromArray() function Feb 20, 2019

vstinner reviewed Feb 20, 2019

View reviewed changes

Include/tupleobject.h Outdated Show resolved Hide resolved

Objects/tupleobject.c Outdated Show resolved Hide resolved

vstinner reviewed Feb 20, 2019

View reviewed changes

sir-sigurd commented Feb 20, 2019

View reviewed changes

vstinner requested changes Feb 21, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 21, 2019

bpo-36030: Add _PyTuple_FromArray() function

bc98990

sir-sigurd force-pushed the tuple-from-array-2 branch from ffbca9e to bc98990 Compare February 24, 2019 16:02

sir-sigurd changed the title ~~bpo-36030: Add PyTuple_FromArray() function~~ bpo-36030: Add _PyTuple_FromArray() function Feb 24, 2019

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 25, 2019

serhiy-storchaka reviewed Feb 25, 2019

View reviewed changes

Remove 'step' parameter

4ba3f4c

vstinner approved these changes Feb 25, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 25, 2019

vstinner added the skip news label Feb 25, 2019

vstinner merged commit 234531b into python:master Feb 25, 2019

bedevere-bot removed the awaiting merge label Feb 25, 2019

sir-sigurd deleted the tuple-from-array-2 branch February 25, 2019 17:19

erlend-aasland reviewed May 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-36030: Add _PyTuple_FromArray() function #11954

bpo-36030: Add _PyTuple_FromArray() function #11954

sir-sigurd commented Feb 20, 2019 •

edited by bedevere-bot

vstinner left a comment

vstinner Feb 20, 2019

sir-sigurd Feb 20, 2019

vstinner Feb 20, 2019

sir-sigurd Feb 20, 2019 •

edited

vstinner Feb 21, 2019

sir-sigurd Feb 20, 2019

vstinner Feb 20, 2019

serhiy-storchaka Feb 25, 2019

vstinner left a comment

vstinner Feb 21, 2019

bedevere-bot commented Feb 21, 2019

sir-sigurd commented Feb 25, 2019

bedevere-bot commented Feb 25, 2019

vstinner commented Feb 25, 2019

serhiy-storchaka left a comment

serhiy-storchaka Feb 25, 2019

vstinner commented Feb 25, 2019

methane commented Feb 25, 2019

vstinner commented Feb 25, 2019

vstinner left a comment

vstinner commented Feb 25, 2019

erlend-aasland May 20, 2021

vstinner May 25, 2021

erlend-aasland May 25, 2021

encukou May 25, 2021

erlend-aasland May 25, 2021

erlend-aasland May 25, 2021

		@@ -1276,45 +1276,19 @@ PyObject_CallFunctionObjArgs(PyObject *callable, ...)
		_Py_NO_INLINE PyObject *
		_PyStack_AsTuple(PyObject const stack, Py_ssize_t nargs)

bpo-36030: Add _PyTuple_FromArray() function #11954

bpo-36030: Add _PyTuple_FromArray() function #11954

Conversation

sir-sigurd commented Feb 20, 2019 • edited by bedevere-bot

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-sigurd Feb 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 21, 2019

sir-sigurd commented Feb 25, 2019

bedevere-bot commented Feb 25, 2019

vstinner commented Feb 25, 2019

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Feb 25, 2019

methane commented Feb 25, 2019

vstinner commented Feb 25, 2019

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-sigurd commented Feb 20, 2019 •

edited by bedevere-bot

sir-sigurd Feb 20, 2019 •

edited