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

Small tweaks with big performance gains #11892

Merged
merged 11 commits into from Jan 18, 2024
Merged

Small tweaks with big performance gains #11892

merged 11 commits into from Jan 18, 2024

Conversation

Rouslan
Copy link
Contributor

@Rouslan Rouslan commented Jan 18, 2024

Subject: Small tweaks with big performance gains

Feature or Bugfix

  • Refactoring

Purpose

  • Increase performance

Detail

I was profiling Sphinx with the Breathe extension (it converts Doxygen output and relies on the cpp domain) on a large amount of code and found that the CPPDomain.resolve_xref method took about 460 seconds. By having ASTDeclaration.get_newest_id cache its output value, this went down to 160 seconds. By implementing ASTIdentifier.__eq__ instead of relying the base class implementation (which loops through __dict__), this went down to 110 seconds. There is no difference in the generated output.

@AA-Turner
Copy link
Member

Hi Rouslan,

Please may you add a CHANGES entry? Do the current tests adequately cover the changes here?

A

@Rouslan
Copy link
Contributor Author

Rouslan commented Jan 18, 2024

I added the entry to CHANGES.

The changes I made shouldn't require any changes to the tests as there should be no difference to the interface of either changed class, with one caveat: the caching of ASTDeclaration.get_newest_id assumes that by the time it is called, no further modifications will be made to that instance of ASTDeclaration. As far as I can tell, this is the case.

@jakobandersen
Copy link
Contributor

Great PR! Your observation about the newest ID is correct, so it's a safe optimization. I started on similar optimizations some time ago, but I never found time to finalize and merge it (master...jakobandersen:sphinx:c_cpp_optimizations). Perhaps there are some things that can be resurrected.

@AA-Turner
Copy link
Member

@jakobandersen I've reflected the changes made here to the C++ domain in the C domain, I expect this will be fine.

A

@AA-Turner AA-Turner merged commit d69d191 into sphinx-doc:master Jan 18, 2024
21 of 22 checks passed
@Rouslan
Copy link
Contributor Author

Rouslan commented Jan 18, 2024

Great, thanks for processing this so quickly!

@AA-Turner
Copy link
Member

If you're able to find other pain points with your profiling work, I'd be more than happy to look to resolve them!

A

@Rouslan
Copy link
Contributor Author

Rouslan commented Jan 19, 2024

I don't plan to spend any more time on this but if you want, you can have the input files I was using for profiling. I uploaded it to OneDrive at https://1drv.ms/u/s!AjEmeqBHiaJDhBSZbQh2CVVA2BJU?e=thAiZq

The data is the Doxygen output from the https://github.com/espressif/esp-idf project. I included a copy of their license in the xml folder where the Doxygen output is, so this should be in compliance with their license.

The Breathe package needs to be installed. I recommend you install the fork I'm working on, which is several times faster: pip install 'breathe@git+https://github.com/Rouslan/breathe.git'

Just uncompress the file and run python3 run_profiler.py to run cProfile and output the results to "profile_results", or run some other profiler on run.py

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants