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

Absolutize paths after remapping #91

Merged
merged 4 commits into from
Jun 27, 2023
Merged

Conversation

keith
Copy link
Member

@keith keith commented Jun 20, 2023

The index store library requires paths being written are absolute. If we
don't absolutize them then it does. Unfortunately its logic for that
computes the PWD for every single path, which can amount to ~5% of the
total time spent in index-import.

Signed-off-by: Keith Smiley keithbsmiley@gmail.com

The index store library requires paths being written are absolute. If we
don't absolutize them then it does. Unfortunately its logic for that
computes the PWD for every single path, which can amount to ~5% of the
total time spent in index-import.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@yongjincho92
Copy link
Contributor

Turns out, we had tons of relative paths for header/source files (.h and .m) and was able to benefit performance from this change, yet again.

Before : ~50 seconds
After : ~30 seconds

Thanks @keith !

index-import.cpp Outdated Show resolved Hide resolved
@keith keith requested a review from brentleyjones June 27, 2023 00:10
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith keith merged commit 431d4e5 into main Jun 27, 2023
2 checks passed
@keith keith deleted the ks/absolutize-paths-after-remapping branch June 27, 2023 23:00
keith added a commit that referenced this pull request Jul 5, 2023
Absolutizing these paths resulted in failures with symlinks downstream.
Resolving those symlinks seems to work but in the case you're trying to
remap from 1 location to another not for Xcode, which rules_swift does
if you use the `swift.use_global_index_store` you actually need the
paths to still be relative in the units.

This reverts commit 431d4e5.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
keith added a commit that referenced this pull request Jul 5, 2023
Absolutizing these paths resulted in failures with symlinks downstream.
Resolving those symlinks seems to work but in the case you're trying to
remap from 1 location to another not for Xcode, which rules_swift does
if you use the `swift.use_global_index_store` you actually need the
paths to still be relative in the units.

This reverts commit 431d4e5.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
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

3 participants