-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Use compressed strings for capsule names. #7255
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
Conversation
For exported C names, it's wasteful to have the name created as Python string in the module state when it's unlikely to be used there. Storing plain C string constants allows the C compiler to store them away in the constant data segment instead. See cython#7107
…s as empty C strings.
|
I think I've squeezed every possible byte out of the storage. It now uses a loop over a function pointer array and a single compressed byte string for the names and the signatures, and compacts the signatures by removing duplicates (which also nicely uses the same signature C string for PyCapsules with the same signature). |
|
To me this makes more sense for the signatures than the names. The signatures are (probably) duplicated while the names aren't. For the names, it saves a little bit of memory making the string-tab array shorter (one pointer per name), but increases the runtime memory since you now have the big unified string plus a separate Python string for each name in the dictionary. For the signatures they can just be a pointer into the big unified string so that's fine. My (possibly wrong) guess would be that this doesn't make much difference to the compression. Only slightly related to this PR but I do think it would be nice to cut down the constant tables to exclude things that are only used when initializing the module - i.e. have a separate shorter-lived table outside the module state for constants that we don't need to keep around forever. |
Yes, that's why it's done that way. I also moved the signatures first so that that part of the aligned byte string can stay in memory or caches, while the names in the back part can reside elsewhere, in unused memory places.
And also the module state. It's now a single string entry for all signatures (which need to stay alive) and all names. I doubt that the names as part of the byte string hurt that much at runtime.
I tried it on
I think that's a good idea. We could also clear out init-time-only constants at the end of |
…st of tuples for the export instead of three separate lists.
|
I think we can probably do the same for the import code. I'll take a look as well. |
My original comment was a little ambiguous. I completely believe in the compression+deduplication of the signatures. I really meant the "compared to a version where the names stay in the module state as they are now". FWIW I had a quick go at that in https://github.com/da-woods/cython/tree/export-without-names. Not really sure how I'd actually compare runtime memory usage - I tried tracemalloc with Cython but don't think it told me too much. |
I added a switch to use either PyBytes or a C string for sig+names and with PyBytes, modules that import the shared module become visibly smaller. So I'll leave the switch at "True".
…nd of ModuleNode.py to keep the ModuleNode class near the top.
|
I integrated also the import code. Looking at the shared module test, the modules that import the shared module become visibly smaller: Before integrating the import code: After: It's interesting that the shared Cython module gets a little larger with this change. Not sure what triggers that. I guess it fails to benefit from the signature deduplication. I also tried using a plain C string for sig+name instead of compressed Python bytes and that makes it much larger (183016 bytes), so that's not a good idea. |
|
It's also interesting that the gain is always exactly 4096 bytes. Might be a segmentation issue. That could also explain why the shared module grows in the test, it might simply move data into a different binary segment that then grows to the next unit boundary. Something like that, maybe? EDIT: This SO question suggests that segments are padded to the page size of the CPU architecture: |
I considered that along the way but I doubt that the names would also be used inside of the module, so that they'd benefit from being Python strings. I'd expect them to be used exactly once in the module, when exporting the capsules. OTOH, we also intern the Python identifiers, and other modules that import the capsules need to provide the same strings, so (globally) interning them on export (and import) could reduce the total amount of different string objects in the Python runtime. That's assuming that the majority of exported names are also used somewhere, which isn't always true for cross package cimports from larger packages (say, SciPy-sized). Here, I'd expect the gain from shared interned strings to be fairly small in comparison to the larger module states. Avoiding interned Python names on both sides (export and import) might actually be a better choice. That written, we're talking about a few kilobytes back and forth here. Whichever we choose probably won't turn a big wheel either way in a real system. If we eventually manage to discard import-time data after use, this will be easy to split up again. With the import integration, I consider the PR in its current state fine for 3.2rc. What do you think? |
I implemented this for the simple case here, but then noticed that this isn't safe because we deduplicate constants (and strings specifically) in the string array. Thus, if a user happens to have a bytes string around that looks like the names or signatures string, then clearing the array index in the module state will kill a user visible string. However unlikely that is, it's easy to get there for user code. That speaks for keeping the strings entirely separate. Definitely not in 3.2 any more. |
Yes - for me I think it'd make most sense to do as a general mechanism rather than adding specific special-cases. (I also don't know exactly what you did here, but the signature strings do need to live as long as the module for the export code so it'd have to be names-only). Will have a thorough look at it later today. |
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.
I can't see any issues.
This feels like the sort of thing that's sufficiently repetitive that it probably doesn't have hidden corner cases so is probably OK for 3.2. But it's always a bit hard to judge
This is now #7266 |
For exported C names, it's wasteful to have each name and signature created as separate Python strings in the module state when it's unlikely to be used there. Storing joined, compacted string constants instead allows using the available byte string compression and reusing signature C strings for identical PyCapsule signatures.
Closes #7107