Skip to content

bpo-46429: Merge all deepfrozen files into one #30572

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

Merged
merged 16 commits into from
Jan 20, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jan 13, 2022

@gvanrossum
Copy link
Member

In the issue you claimed a 1.2 Mb space savings. How did you measure that?

On MacOS I see ~0.2 Mb space savings (both size and ls show roughly this).

@kumaraditya303
Copy link
Contributor Author

In the issue you claimed a 1.2 Mb space savings. How did you measure that?

Yes, it was 1.2 megabits not 1.2 megabytes, so around ~0.2 MB is correct.

@kumaraditya303
Copy link
Contributor Author

@gvanrossum What's next step for this PR, I mean do you think this would be merged or was this just for experimentation ?

@gvanrossum
Copy link
Member

I still need to review it. Maybe @tiran could also have a look?

@tiran
Copy link
Member

tiran commented Jan 18, 2022

If I understand the change correctly then we are no longer tracking dependencies correctly. Does a change to Lib/codecs.py trigger a rebuild of Python/frozen_modules/codecs.h and Python/deepfreeze/deepfreeze.c?

@kumaraditya303
Copy link
Contributor Author

If I understand the change correctly then we are no longer tracking dependencies correctly. Does a change to Lib/codecs.py trigger a rebuild of Python/frozen_modules/codecs.h and Python/deepfreeze/deepfreeze.c?

Yes, fixed now.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Basics look good, I have some nits to pick. You can undraft this.

@gvanrossum
Copy link
Member

Hey Kumar, I wonder if (in a separate PR?) you'd be interested in implementing another idea to make the generated C code more understandable. For string constants, we could easily generate names that are more readable than zipimport_toplevel_consts_21_varnames_21, by taking the first N characters, replacing non-alphanumeric characters with '', and seeing if the result doesn't contain too many '' characters, and adding some hash to ensure uniqueness. So then that variable could become e.g. const_str_12aafd40_comment_size. We could even do similar things for tuples and numbers. What do you think? This wouldn't change the performance of the code, but it would make things a little more readable when idly browsing the generated file.

@gvanrossum
Copy link
Member

Can you also open a bpo issue and add a news blurb?

@kumaraditya303 kumaraditya303 changed the title WIP: Merge all deepfrozen files into one bpo-46429: Merge all deepfrozen files into one Jan 19, 2022
@kumaraditya303 kumaraditya303 marked this pull request as ready for review January 19, 2022 04:28
@kumaraditya303 kumaraditya303 requested a review from a team as a code owner January 19, 2022 04:28
@kumaraditya303 kumaraditya303 removed the request for review from a team January 19, 2022 05:07
@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jan 19, 2022

Hey Kumar, I wonder if (in a separate PR?) you'd be interested in implementing another ...

@gvanrossum I am interested, will create another PR.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Just one request, fold the long line in the Windows project file.

@gvanrossum
Copy link
Member

Oh, there was one more request from the Faster CPython team. Could you run the PyPerformance benchmark (in PGO mode, on as quiet a machine as possible) before and after this to see if there's a net perf benefit? (We'd expect a small improvement due to the memory savings, but there could be a slight negative effect due to less memory locality.)

@kumaraditya303
Copy link
Contributor Author

With patch:

0.011 ./python

Without patch:

0.011 ./python

@gvanrossum
Copy link
Member

What benchmark did you run? Definitely not PyPerformance!

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jan 20, 2022

Nope https://github.com/python/cpython/blob/main/Tools/scripts/startuptime.py

I didn't see you asked for pyperformance (sorry).

@gvanrossum
Copy link
Member

Yeah, I trust that the startup time is the same or better (actually the precision of that number is insufficient, we should print another digit), but we were wondering about effects on other benchmarks due to locality in deep-frozen modules. It's the only thing left to do here.

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jan 20, 2022

Yeah, I trust that the startup time is the same or better (actually the precision of that number is insufficient, we should print another digit), but we were wondering about effects on other benchmarks due to locality in deep-frozen modules. It's the only thing left to do here.

Perhaps, you can verify it if you have a benchmark setup ? I don't expect any significant change but less memory use.

@gvanrossum
Copy link
Member

Okay, I'm just going to merge. We can watch speed.python.org tomorrow.

@gvanrossum gvanrossum merged commit ef3ef6f into python:main Jan 20, 2022
@kumaraditya303 kumaraditya303 deleted the deepfreeze branch January 20, 2022 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants