Skip to content

Optimize Path#relative_to?#15594

Merged
straight-shoota merged 2 commits intocrystal-lang:masterfrom
HertzDevil:perf/path-relative-to
Mar 26, 2025
Merged

Optimize Path#relative_to?#15594
straight-shoota merged 2 commits intocrystal-lang:masterfrom
HertzDevil:perf/path-relative-to

Conversation

@HertzDevil
Copy link
Contributor

Path#relative_to? calls #/ to build up the final path successively. This is done for each path component, which means the whole method has quadratic complexity. Only a single #join is necessary here and reduces the complexity to linear.

@straight-shoota
Copy link
Member

straight-shoota commented Mar 24, 2025

Path#join(Enumerable) still results in a sequence of individual join calls to construct the final path.
So it doesn't seem this change would really improve much. It just introduces an intermediary array.

We know all parts are well-formed and they have no separators at either end. So we could probably use a String::Builder here and trivially concatenate the parts. We do that in #normalize already.

@straight-shoota
Copy link
Member

Now it makes more sense 👍
I'm still wondering if a string builder wouldn't be better. With the array, we know the exact size of the string and don't need to reallocate the string buffer. But that comes at the cost of allocations for the array itself.
If we could calculate a good estimation for an upper bound of the string size, the string builder should be more efficient.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Regardless of further improvements, this seems like a nice, low hanging fruit.
Of course we could do even much more by stepping through the original paths (non-normalized) directly, without iterators. Could also memcpy the trailing part of target_path.

@straight-shoota straight-shoota added this to the 1.16.0 milestone Mar 24, 2025
@straight-shoota straight-shoota merged commit 7ad5ed5 into crystal-lang:master Mar 26, 2025
36 of 38 checks passed
@HertzDevil HertzDevil deleted the perf/path-relative-to branch March 26, 2025 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants