-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-139353: Add Objects/unicode_writer.c file #139911
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
Move the public PyUnicodeWriter API and the private _PyUnicodeWriter API to a new Objects/unicode_writer.c file. Rename a few helper functions to share them between unicodeobject.c and unicode_writer.c, such as resize_compact() or unicode_result().
a263030 to
c2f600e
Compare
|
As mentioned before, I don't think turning these parts into separate object files is a good idea. Have you benchmarked the effect of putting the writer into a separate object file vs. keeping it in the unicodeobject.c file (vie #includes ) |
I'm not sure if it's relevant since I ran a benchmark on ref is the main branch and split is this PR:
Benchmark hidden because not significant (1): repr tuple-5 Note: I didn't use LTO+PGO optimizations which should reduce the noise even more. import pyperf
runner = pyperf.Runner()
for size in (1, 5, 10, 50, 100):
runner.timeit(f'repr tuple-{size}',
setup=f't = (1,)*{size}',
stmt='repr(t)') |
|
Thanks for running the benchmark. I'm more concerned about uses of unicodeobject.c code in this new unicode_writerr.c, than use of the writer APIs in unicodeobject.c. |
Oh ok. Well, it has been measured by the benchmark as well. |
|
@serhiy-storchaka @malemburg: So are you against merging this PR, or can I merge it? |
|
Fine for me. I would have thought that the benchmarks would show a degradation in performance due to the compiler not being able to inline often used functions, but since that's apparently not the case, I don't have objections. Please do run such benchmarks for future splits out of Thanks. |
serhiy-storchaka
left a comment
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.
LGTM. 👍
This is only 600 lines, to moving them out will not have large benefit. But this can be good for source code structuring purpose.
|
Merged, thanks for reviews. |
Move the public PyUnicodeWriter API and the private _PyUnicodeWriter API to a new Objects/unicode_writer.c file.
Rename a few helper functions to share them between unicodeobject.c and unicode_writer.c, such as resize_compact() or unicode_result().