Skip to content

bpo-36904: new function _PyStack_DictAsVector #13308

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

Closed
wants to merge 1 commit into from

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented May 14, 2019

Provide _PyStack_DictAsVector as safe and more efficient replacement for _PyStack_UnpackDict.

https://bugs.python.org/issue36904

https://bugs.python.org/issue36907

@jdemeyer
Copy link
Contributor Author

skip news because this is an implementation detail

CC @encukou @markshannon @scoder @vstinner

@jdemeyer
Copy link
Contributor Author

I'm wondering whether we should use a special tuple subclass for this. It wouldn't really change much in practice, but it would feel more justified to hack with the tuple size.

}
/* Truncate the tuple to the desired length */
Py_SIZE(kwtuple) = nkwargs;
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't look safe at all. Won't such a tuple join the wrong freelist in tupledealloc?

@jdemeyer
Copy link
Contributor Author

I didn't think of the freelist but it's true that I already had bad feelings about messing with the tuple size like that. What do you think of my proposal (see above) of making a special tuple subclass for this?

@encukou
Copy link
Member

encukou commented May 17, 2019

For a good answer I'd need to write the code :)
Generally, this sounds like an optimization, so consider doing it in two steps:

  • Fix the issue simply and robustly (seems to need two allocations and a bunch of refcount poking)
  • Afterwards, optimize and measure

@jdemeyer
Copy link
Contributor Author

For a good answer I'd need to write the code :)

write the code or just see the code (written by me for example)?

Generally, this sounds like an optimization, so consider doing it in two steps:

* Fix the issue simply and robustly (seems to need two allocations and a bunch of refcount poking)

* Afterwards, optimize and measure

This two-step process is going to take more work: it requires to discuss two API designs and implementations instead of one (with the first one going to be thrown away anyway once we do the second).

So, I'd like to do it in one step if possible.

@jdemeyer
Copy link
Contributor Author

* bunch of refcount poking

On the other hand, adding the "refcount poking" part is pretty simple. So if you agree with doing just that manually (without changing _PyStack_UnpackDict at all), then I'm OK with that.

@encukou
Copy link
Member

encukou commented May 17, 2019

write the code or just see the code (written by me for example)?

For a relatively complex optimization? benchmark the code, before and after.

This two-step process is going to take more work: it requires to discuss two API designs and implementations instead of one (with the first one going to be thrown away anyway once we do the second).

But without the first, it'd be hard to argue that the tuple subclass actually brings any benefit. Tuple subclasses behave quite differently from exact tuples (they don't use freelists, for one).

On the other hand, adding the "refcount poking" part is pretty simple. So if you agree with doing just that manually (without changing _PyStack_UnpackDict at all), then I'm OK with that.

Definitely! All I ask for before the optimization is a correct baseline.

It might then very well turn out that the optimization is unnecessary.

@encukou encukou changed the title bpo-36907: new function _PyStack_DictAsVector bpo-36904: new function _PyStack_DictAsVector May 22, 2019
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jul 2, 2019

I decided to drop this idea. It's more controversial than I initially thought and I see why (I can't disagree).

For reference: #14517 optimized _PyStack_UnpackDict in other ways, in particular to create a fast path in the case of no keyword arguments.

@jdemeyer jdemeyer closed this Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants