asdf icon indicating copy to clipboard operation
asdf copied to clipboard

perf: speed improvements

Open stephanGarland opened this issue 2 years ago • 7 comments

Summary

This is a draft for now. It pulls in the patch I linked here, as well as some other work. I mostly wanted to get people's opinion on it early before I continue with the entire project.

In general, it speeds up performance by reducing fork calls, either by combining various combinations of sed | grep | cut etc. into a single awk call, or where possible, using shell built-ins like parameter substitution and modification.

Fixes

It addresses but does not fix issue 290

Other Information

I haven't ran the full test suite yet, but I know at least one (where) fails; I believe this to be a false positive, and will address it.

stephanGarland avatar Jan 20 '23 04:01 stephanGarland

@stephanGarland A few thoughts after a quick glance.

Before we introduce changes like these I would like to standardize and have in our CI pipelines performance testing and reporting to PRs. That way we identify the actual performance uplift of a specific change. (https://github.com/sharkdp/hyperfine is my current candidate for this automation. How to integrate it is something I will look at very soon).

Once we have the above, we can then go through and add each of these changes one at a time. That way we should have clear data about the improvement for each case.

Further, #1433 & #1138 are partially addressing #1396 & #1397 which will lead us down the path towards using more native shell features. I am not sure if changes for those will clash with what you have here, but be prepared for some rework here.

This draft PR is a great place for us to discuss your performance improvements and ideas, so please keep this open, keep updating it, and I will hopefully have time to review the rest of it soon.

Thanks! This is exciting work!

jthegedus avatar Jan 20 '23 06:01 jthegedus

Awesome! Performance increases are a much needed fix.

I know #1433 was mentioned, and as its author, I know that PR is pretty big, but I only fixed formatting (mostly), for only files in test/*, so there should be little to no conflicts. Same with the other PR.

I've identified some Bash incompatibilities, and some possible improvements. There might be some disagreement of some sort about the first suggestion, so maybe that one can be hashed out once the mentioned additional perf test is integrated in the CI pipeline

hyperupcall avatar Jan 22 '23 09:01 hyperupcall

Before we introduce changes like these I would like to standardize and have in our CI pipelines performance testing and reporting to PRs.

For the record I agree with @jthegedus on this. I'd love to improve performance but I also don't want to be reviewing changes that only improve performance by a small percentage, or changes that only improve things for a small subset of users (not saying that is the case here, I'm just now looking at these changes).

Stratus3D avatar Jan 27 '23 18:01 Stratus3D

Once I finish my other PRs I will work on the performance automation for this repo.

jthegedus avatar Jan 31 '23 12:01 jthegedus

I found and fixed a bug I had introduced with parse_asdf_version_file that was causing a ton of failures. There are still test failures (34/352, but tbf some of those are from fish and elvish simply because I don't have them installed yet), but the number is greatly reduced.

stephanGarland avatar Feb 10 '23 17:02 stephanGarland

Happy 1 year anniversary for this PR! Sic.

ssbarnea avatar Jan 21 '24 21:01 ssbarnea

Happy 1 year anniversary for this PR! Sic.

I got frustrated trying to fix bugs only present in esoteric shells like elvish, and around the same time, shifted to mise (née rtx). Sorry. I might pick it up again once my professional life has calmed down some.

stephanGarland avatar Apr 28 '24 16:04 stephanGarland