CodSpeed Instrumentation Performance Report

Merging #12525 will improve performances by 3.43%

_{Comparing taearls:07-08-refactor_ast_remove_astkind_for_argument_ (5a7cd39) with main (91c88e2)¹}

Summary

⚡ 1 improvement
✅ 12 untouched
⏩ 24 skipped²

Benchmarks breakdown

Very glad to see you're still on the case with this! Looks like it's a long hard slog. Please shout if you need any help.

Very glad to see you're still on the case with this! Looks like it's a long hard slog. Please shout if you need any help.

yes it has been pretty challenging to be honest but I'm still chipping away at it when I can!

the draft PR is most of the way there. right now I'm working on minimizing the changes that were done to the linting rules.

the main challenge at this point is getting the formatter benchmarks not to degrade. right now my short term goal is to finish everything aside from that.

in terms of performance:

My initial findings are that removing AstKind::Argument made the argument detection in the formatter turn from an O(1) operation (looking directly at the AstKind::Argument wrapping the current node being processed) to an O(n) operation (checking for span equality in the argument list of the parent).

I've tried implementing a few refactors to improve the performance, but it has either made it worse or introduced regressions in the prettier conformance tests, so I've reverted those changes on this branch.

I think I just need to find a way to reduce the redundant argument checks that currently exist on my branch, but I haven't found a solid way to do that yet.

Sorry I'm slow to come back to you. Have been away.

This PR has diverged a long way from the original #12127. At a time that's convenient to you, would you like to squash all the changes (including those from #12127) and rebase on latest main? That would make the diff rather easier to follow.

Maybe I'm being naive, but I don't think the perf impact should be too bad - as far as I can see, you should only need one more iteration up the ancestry chain to determine if in an Argument position or not.

I may be able to offer some pointers if I can see more clearly where you're up to (but feel free to ignore me if you don't need/want any help!)

Sorry I'm slow to come back to you. Have been away.

This PR has diverged a long way from the original #12127. At a time that's convenient to you, would you like to squash all the changes (including those from #12127) and rebase on latest main? That would make the diff rather easier to follow.

Maybe I'm being naive, but I don't think the perf impact should be too bad - as far as I can see, you should only need one more iteration up the ancestry chain to determine if in an Argument position or not.

I may be able to offer some pointers if I can see more clearly where you're up to (but feel free to ignore me if you don't need/want any help!)

No worries at all! I understand. I appreciate you checking in.

I rebased off the latest main. The diff is quite large in this PR because it's pointed towards #12127 which I think is pretty far behind main. for now I changed the target branch on this PR to main to simplify things, but let me know if you want me to change it back! it should be much easier to see the diff now.

If you have bandwidth, I think I could use some help with the performance. I thought the same, that it would just take one iteration up the AST tree to get the argument context. I'm not sure if I may have done something silly in the formatting code changes. While patching the conformance tests between this branch and main I made some changes to the argument detection logic that may not have been the best.

I've tried a few different strategies to fix the performance, and I've learned a few things:

• pre-allocations, even if they make performance better on my machine, make the benchmark results worse because of the memory-constrained machines that are emulated in CodSpeed
• early returns for certain conditions (like not having arguments) helps performance but isn't enough.

Ok, after switching the target branch to main, the benchmarks appear to be resolved now. I probably should've done this sooner!

the only thing I'm noticing is that the prettier conformance tests have regressed a bit. once that's addressed I think this PR should be good to review. I'll start on it today

Are you already working on fixing the formatter tests? If not, leave it to me, and I will use a simple way to avoid a bunch of formatter tests from failing.

Are you already working on fixing the formatter tests? If not, leave it to me, and I will use a simple way to avoid a bunch of formatter tests from failing.

yeah, I had started working on it on a separate branch until I finished, but if you have a better way of doing it, by all means go ahead!

Are you already working on fixing the formatter tests? If not, leave it to me, and I will use a simple way to avoid a bunch of formatter tests from failing.

as an update, in my separate dev branch I have fixed the typescript conformance tests, and have 14 javascript conformance tests to go. I'm doing it on a separate branch so that I don't commit a bunch of noisy debug logs and such as I continue to iterate on it. I'm gonna keep iterating on this but lmk if you want me to push up my progress so far.

as an update, in my separate dev branch I have fixed the typescript conformance tests, and have 14 javascript conformance tests to go. I'm doing it on a separate branch so that I don't commit a bunch of noisy debug logs and such as I continue to iterate on it. I'm gonna keep iterating on this but lmk if you want me to push up my progress so far.

Great job, can you send a PR? Btw, I am cleaning up the formatter, so I am concerned this will make a lot of conflict.

as an update, in my separate dev branch I have fixed the typescript conformance tests, and have 14 javascript conformance tests to go. I'm doing it on a separate branch so that I don't commit a bunch of noisy debug logs and such as I continue to iterate on it. I'm gonna keep iterating on this but lmk if you want me to push up my progress so far.

Great job, can you send a PR? Btw, I am cleaning up the formatter, so I am concerned this will make a lot of conflict.

yes! I'll try to do that later tonight or tomorrow morning.

I'm doing it on a separate branch so that I don't commit a bunch of noisy debug logs and such as I continue to iterate on it.

Please don't worry about that. Not a problem. Of course, it's up to you how you want to work, but personally I think keep it simple! One PR/branch is enough...

I'm doing it on a separate branch so that I don't commit a bunch of noisy debug logs and such as I continue to iterate on it.

Please don't worry about that. Not a problem. Of course, it's up to you how you want to work, but personally I think keep it simple! One PR/branch is enough...

Yup that makes total sense. I also use the separate branch during active dev so I can guard against Claude doing anything unexpected to the branch I've got a PR attached to. My intention was to only have one PR in the end.

I'll push up my updates in the morning. There's only a few conformance tests left to fix 🙂

Please don't feel under any pressure to rush. Just to give you a heads-up: this PR is large, and from past experience changes to AstKind can be quite subtle. So it'll likely need careful review, and most of the core team are quite tied up over next few weeks. So it may take some time to get merged.

That's not to say your work isn't appreciated. It is very much. Just the timing is a bit tricky.

pushed up my changes so far. a lot of the conformance tests are fixed. there are some snapshot updates to work through to get the CI passing again. I'll try to get to that this weekend!

I fixed the merge conflicts, but there are a lot of new CI failures now. I'm digging into it. I merged the latest from main into this branch and now the file count for the PR has exploded as well. I'll see what I can do.

ugh, crap. I did not mean to close this. one sec.

new PR here #13902. sorry about that.