perf: cache Documents relative path

[RoloEdits]

helix_stdx::path::get_relative_path was always performed for the statusline render. This PR now caches the relative path in Document instead.

(Highlighted in purple: statusline::render)

Before:

After:

[NikitaRevenco]

how do you generate this kind of graph?

[RoloEdits]

I used https://github.com/flamegraph-rs/flamegraph

[archseer]

Good catch!

I'm not sure if all of these should be labeled inline though, most of these functions are quite large (e.g. normalize) and rust will already use it's own inlining heuristics, #[inline] usually signals to inline across crate boundaries.

I'd only mark private functions in this package as inline.

[RoloEdits]

get_truncated_path, for instance, is only used in one spot:

Should I keep these that have low (<3) cross crate usages? Or remove it from all pub fn?

(just checked the rest, almost all of them are only used in one spot)

[RoloEdits]

rust will already use it's own inlining heuristics

If I recall correctly, it doesn't inline across crate boundaries without the attribute. Meaning these one spot usages that could very well be inlined wouldnt be able to without it.

[RoloEdits]

Also as far as improvement, its hard to say, but the samples for the statusline::render accounted for 25% of the time in the before and 0.4% after. For the sampling here I didnt do anything, but in other examples of when I did and sampled, I found that the size of those operations in the screenshots ended up accounting for about half the total samples. So I'd say ~8-10% reduction is total sample appearances.

[the-mikedavis]

I would prefer to drop the inlining for simplicity. From what I've read it seems like inlining can have unpredictable performance effects so it's always worth trying to measure the improvement from inlining. None of these functions should be called in a very tight loop so I would doubt that inlining has a significant impact in this case.

[RoloEdits]

Its not that it forces inlining, thats #[inline(always)], it just allows the optimizing compiler to decide if it should or not, rather than always not. In the case of single use functions, it allows what would always be a function call, and an instruction cache miss, to be moved locally in the calling code.

I can remove them if that's the final decision though.

[the-mikedavis]

By "inlining" I mean #[inline] or #[inline(always)]. I just doubt it makes any significant difference for these functions

[TornaxO7]

Should I keep these that have low (<3) cross crate usages? Or remove it from all pub fn?

(just checked the rest, almost all of them are only used in one spot)

Lol, I was a bit irritated, because I read <3 as a heart xD

However, in my opinion it isn't important how often a function is used, if the readability improves.
I'd prefer a function which describes it's doing in its name instead of having to read through multiple lines which are (maybe) commented. The compiler is often smart enough to inline it himself.

Its not that it forces inlining, thats #[inline(always)], it just allows the optimizing compiler to decide if it should or not, rather than always not.

No, the compiler itself will inline code, if its heuristics approve it (assuming we are in release mode) even if you didn't explicitely apply the #[inline] annotation. You can see inline hints rather for special cases but for most of the time, they are unnecessary.

[RoloEdits]

However, in my opinion it isn't important how often a function is used, if the readability improves.
I'd prefer a function which describes it's doing in its name instead of having to read through multiple lines which are (maybe) commented.

Not sure what adding an attribute has to do with naming of functions?

No, the compiler itself will inline code

Yes, in the same crate. rustc does not inline cross crate without the attribute, and this is recursive. Which is why I also removed the attribute from the private functions, as if no pub functions have it, its pointless.

https://matklad.github.io/2021/07/09/inline-in-rust.html#Inlining-in-Rust

[TornaxO7]

Not sure what adding an attribute has to do with naming of functions?

Well I though you wanted to remove functions because they are only used once as described here: #12385 (comment)
But maybe I've misunderstand you. Sorry.

[RoloEdits]

Well I though you wanted to remove functions

Ah, I see where that could have been mixed up. Yeah, I meant that because the functions were only used in one spot that it could have been something the optimizer could just inline if it had decided so, if it had the capability to do so with the attribute(cross crate: helix-stdx -> helix-*).

I do see another area where these could be improved though, particularly the larger functions. As they are generic over AsRef, the function could be generated more than once due to monomorphization, as it could take a &PathBuf, PathBuf, or even a String(or &str) for example.

The function body could be lifted out to an impl function that just takes a Path and then have the generic impl_fn(path.as_ref()) so that the larger body of the function isnt generated more than once. But I dont think code generation here would have any outsized impacts(though its something std does often, and since its a stdx could argue it should have same treatment?).

[RoloEdits]

Should I remove stdx::path changes from here? Still some typo/doc changes that were made when I was "touching" those. But with the changes now, I am not.

[the-mikedavis]

Thanks! Nice to get better encapsulation alongside a performance improvement 👍

[the-mikedavis]

I think those are fine, the stdx path module is fairly new and I don't think is touched in any open PRs