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

Optimize determine_rpath_dirs #14207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bruchar1
Copy link
Member

When generating targets in ninja backend for my project, determine_rpath_dirs is the second most expensive function call. Here are some optimizations, mostly replacing expensive regex with string manipulation.

@bruchar1 bruchar1 requested a review from jpakkane as a code owner January 29, 2025 21:45
Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

mostly replacing expensive regex with string manipulation.

Can we get effectively the same benefit by checking the prefix and returning early, without rewriting the regex? The code is longer, harder to read and less self-documenting now.

Alternatively what about an lru_cache? This code seems to be run constantly but only have two valid answers: one answer for each for_machine.

@bonzini
Copy link
Collaborator

bonzini commented Jan 30, 2025

Alternatively what about an lru_cache? This code seems to be run constantly but only have two valid answers: one answer for each for_machine.

Yeah, there's a lot of cleanup possible here:

  • get_rpath_dirs_from_link_args() should be a method on the compiler, rather than a static method in the backend. The current implementation is only valid for compilers that support -Wl, and other compilers should just return [].
  • get_external_rpath_dirs should take the machine instead of the target and it should be lru_cached. Possibly it should be a method in coredata rather than the Backend.

@bruchar1 bruchar1 marked this pull request as draft January 30, 2025 12:27
@bruchar1
Copy link
Member Author

Thank you for your good suggestions. I will work on it!

@bruchar1 bruchar1 force-pushed the optimize-determine-rpath-dirs branch from 1bcd3d1 to 788a761 Compare January 30, 2025 20:54
@bruchar1
Copy link
Member Author

My profile now tells me 3.4 s instead of 9.2 s for the determine_rpath_dirs function when generating my project.

@bruchar1 bruchar1 marked this pull request as ready for review January 30, 2025 20:58
@bruchar1 bruchar1 added this to the 1.8 milestone Jan 31, 2025
When generating targets in ninja backend for my project,
`determine_rpath_dirs` is the second most expensive function
call. Here are some optimizations:

- Use a single regex instead of 3 to find rpath args
- Add cache to get_rpath_dirs_from_link_args
- Remove unneeded isinstance check for get_external_link_args
- early exit when iterating for rpath arguments
- compute system_dirs and external_rpaths once per target instead
  of recomputing them for every link args of the target (!!!)
@bruchar1 bruchar1 force-pushed the optimize-determine-rpath-dirs branch from 788a761 to 90c8ff2 Compare January 31, 2025 14:49
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.

3 participants