Skip to content
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

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

Merged
merged 16 commits into from Jan 20, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jan 13, 2022

@gvanrossum
Copy link
Member

gvanrossum commented Jan 14, 2022

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

kumaraditya303 commented Jan 14, 2022

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

kumaraditya303 commented Jan 18, 2022

@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

gvanrossum commented Jan 18, 2022

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

kumaraditya303 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?

Yes, fixed now.

Copy link
Member

@gvanrossum gvanrossum left a comment

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

Makefile.pre.in Outdated Show resolved Hide resolved
Makefile.pre.in Outdated Show resolved Hide resolved
Tools/scripts/deepfreeze.py Outdated Show resolved Hide resolved
Tools/scripts/deepfreeze.py Outdated Show resolved Hide resolved
Tools/scripts/deepfreeze.py Outdated Show resolved Hide resolved
Tools/scripts/deepfreeze.py Outdated Show resolved Hide resolved
Tools/scripts/deepfreeze.py Outdated Show resolved Hide resolved
Tools/scripts/deepfreeze.py Outdated Show resolved Hide resolved
Tools/scripts/deepfreeze.py Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

gvanrossum commented Jan 18, 2022

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

gvanrossum commented Jan 18, 2022

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 Jan 19, 2022
@kumaraditya303 kumaraditya303 requested a review from a team as a code owner Jan 19, 2022
@kumaraditya303 kumaraditya303 removed the request for review from a team Jan 19, 2022
@kumaraditya303 kumaraditya303 requested a review from gvanrossum Jan 19, 2022
@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

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

PCbuild/_freeze_module.vcxproj Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

gvanrossum commented Jan 19, 2022

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

kumaraditya303 commented Jan 20, 2022

With patch:

0.011 ./python

Without patch:

0.011 ./python

@gvanrossum
Copy link
Member

gvanrossum commented Jan 20, 2022

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

gvanrossum 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.

@kumaraditya303 kumaraditya303 requested a review from gvanrossum Jan 20, 2022
@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

gvanrossum commented Jan 20, 2022

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
12 checks passed
@kumaraditya303 kumaraditya303 deleted the deepfreeze branch Jan 20, 2022
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.

None yet

5 participants