-
Notifications
You must be signed in to change notification settings - Fork 782
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
RFC: Add intern! macro which can be used to amortize the cost of creating Python objects by storing them inside a GILOnceCell. #2269
Conversation
Perhaps we should consider #2223 |
465c371
to
ba45a44
Compare
After reading that issue, I am not what the proposed action would be. Isn't the Additionally, I do like that |
I worded that poorly. I meant more "this PR and that issue are closely related and should be considered together". I would much prefer this macro over messing with internal cpython api. @alex what are your thoughts on this? |
I'm concerned with how this interacts with generics: pub fn bar<T: pyo3::ToPyObject>(py: Python<'_>, thing: T) -> &PyAny {
intern!(py, &thing)
}
fn main() {
Python::with_gil(|py| {
let a = bar(py, "foo");
let b = bar(py, 42);
assert!(a.is(b));
});
} I think one way to fix this is to not allow the macro to capture variables...let me experiment with that. |
c3603f9
to
ad1d380
Compare
You mean something like forcing the initializer to be evaluated in const context? |
ad1d380
to
a4629d7
Compare
A few thoughts:
|
a4629d7
to
c0fc940
Compare
Amended the commit with a simple benchmark and temporarily included your
|
Nice. While I'm sad to give up 2ms this is a HUGE improvement over the status quo :-) |
And has the advantage that it won't be removed soon like |
c0fc940
to
de19b4c
Compare
I was thinking about this and started to write a Rust-side re-implementation of what CPython 3.8 does for
(The absolute numbers are different because I remembered to disable CPU frequency boosting.) So I think we should - if only for this reason - expose a macro limited to creating Python strings for identifiers and make sure that we intern those strings. (The bespoke re-implementation of
|
…Python objects by storing them inside a GILOnceCell.
de19b4c
to
bfbef75
Compare
bfbef75
to
791e388
Compare
This is very cool! The only concern I have, is that I think for PEP 489 compatibility (reloadable modules / sub-interpreter compatibility) we need to be moving away from global statics to module state. I haven't thought about it super hard though; so I'm not sure if this is ok as-is or whether there's an easy tweak to intern as part of module state? |
From the pep:
It's a bit weasily but it sounds like putting immutable builtin types like integers and strings in statics is OK? Also, does this mean the GilOncecell example is a bad idea? |
I think so too. Another good reason to restrict this to strings.
Yes, we probably should offer a convenient way to attach such "static" globals to modules. |
I am not sure this applies to interned strings as well as those implicitly reference the interning table that as probably at least per-interpreter?
I suspect that the result might end up looking like the standard library's
I admittedly do not yet know how a PyO3 that is properly compatible with both of these things will look like, but I suspect that |
…addition to the reference.
791e388
to
f777372
Compare
Agreed, once we've figured this out there will likely be a bunch of APIs which would need deprecation anyway. |
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.
Thanks :) Just one little thing...
Sorry for the late change, but while trying this out, I noticed that implicitly using |
fe5bf95
to
30d414f
Compare
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 change to expr
is a bit footgunny, I think.
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.
Thanks. This should be ready to merge now? 👍
Think so 👍 |
LGTM -- should we make a list somewhere of "things that might need to
change when we get to multiple module instantion"?
…On Mon, Apr 4, 2022 at 2:54 PM David Hewitt ***@***.***> wrote:
Think so 👍
—
Reply to this email directly, view it on GitHub
<#2269 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBEFMR7VPP6RK26EY43VDM3F3ANCNFSM5SM7XIZQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
…ing multiple times them with different text values yielding inconsistent results.
0958a78
to
89577a2
Compare
Basically turns the proposed solution of #2266 into a discoverable/reusable macro.
I think this should be mentioned in the guide to actually be discoverable, but I am not sure where.
Closes #2223