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

Document PGO in devguide #1153

Merged
merged 19 commits into from
Aug 13, 2023
Merged

Document PGO in devguide #1153

merged 19 commits into from
Aug 13, 2023

Conversation

KilJaeeun
Copy link
Contributor

@KilJaeeun KilJaeeun commented Aug 11, 2023

Hi! this is my first open source Pull Request in CPython Sprint at PyCon KR so it might be awkward. Added description of enable optimization option. Please take good care of me!
image


📚 Documentation preview 📚: https://cpython-devguide--1153.org.readthedocs.build/

@corona10
Copy link
Member

@hugovk @CAM-Gerlach @AlexWaygood

Dear reviewers,
@KilJaeeun is a participant in the PyCon KR sprint.
This is the first PR to the OSS world in her life ever!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks, it's really useful to document this! This looks pretty good.

Could you maybe start the section with a brief description of why you might or might not want to use this option? I worry that at the moment, people new to Python might think that this is the best option that they should use all the time (since "optimized" is a very positive adjective!)

Maybe the section could start with something like this?

If you are trying to improve the performance of Python, you will probably want to use an optimized build of CPython. It can take a lot longer to build CPython with optimizations enabled, and it's usually not necessary to do so. However, it's essential if you want accurate benchmark results for a proposed performance optimization.

Could you also please wrap each line to 80 characters, like the other paragraphs in this document?

@AlexWaygood
Copy link
Member

Closing and reopening to retrigger the CLA bot

@AlexWaygood AlexWaygood reopened this Aug 11, 2023
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Aug 11, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@KilJaeeun
Copy link
Contributor Author

@corona10 @AlexWaygood
I reflected the code review Please check it out!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks, nearly there! A few more small suggestions below. If you like my suggestions, you can apply them by clicking the "commit suggestion" button below each suggestion :)

getting-started/setup-building.rst Outdated Show resolved Hide resolved
getting-started/setup-building.rst Outdated Show resolved Hide resolved
getting-started/setup-building.rst Outdated Show resolved Hide resolved
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Depending on how much detail you want to go into, you could link the specific options themselves with :option`python:--enable-optimizations` or :option`python:--with-lto`.


Related, I was a little surprised to see configure options in d.p.o -- thinking about it, should we consider moving it to the devguide? I'm not sure many end-users build their own Python from sources...

A

getting-started/setup-building.rst Outdated Show resolved Hide resolved
getting-started/setup-building.rst Outdated Show resolved Hide resolved
KilJaeeun and others added 5 commits August 13, 2023 21:14
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

.idea/workspace.xml Outdated Show resolved Hide resolved
@KilJaeeun
Copy link
Contributor Author

@AlexWaygood
thank you for

you can apply them by clicking the "commit suggestion" button below each suggestion :)
this mention! i didn't know that.. thank you!!!

@AA-Turner
I didn't know how to specify detailed options, so I was thinking about it, but thank you. That's what I was looking for!

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Looks good overall! Thanks @KilJaeeun and again congratulations on your first PR!

A

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@AlexWaygood AlexWaygood dismissed corona10’s stale review August 13, 2023 12:47

The requested changes were made

@AlexWaygood AlexWaygood merged commit d994dff into python:main Aug 13, 2023
2 checks passed
@AlexWaygood
Copy link
Member

Thanks again @KilJaeeun, this was a great first PR!

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.

4 participants