Start removing all the clones

[savente93]

So far to get a prototype going my strategy has just been "keep owned copies everywhere and just clone literally anything". This is fine but a little inefficient. As things start to mature I'd like to see where we can do with slices, and find other ways to not clone as much.

Might be nice to do this after we setup some benchmarks so we can see how much of a difference it makes.

[savente93]

This is definitely a larger issue, and I'm adding the mentoring available label. If anyone wants to work on this issue they definitely don't have to resolve all of it (in fact I'd discourage that). It's not as worked out, therefore I haven't added the good first issue, but I'm happy to sit with folks and help them pick a place to get started.

[oopscompiled]

hi, I assume you are thinking of refactoring the string logic into something like Cow, which would reduce redundant string allocations and already implements Deref. I’d be happy to help work on it

[savente93]

Hi! yeah, the point is definitely to improve performance/decreace memory footprint, although I'm sort of just assuming that will improve. I don't have any bench marks to back it up yet. However, if we can do it without making the APIs significantly worse there's also no downside to doing it, so we may as well. I haven't planned this part out in detail yet, so I'd suggest starting with a small part and just "moving the clone barrier back" if you get what I mean.

Either Cow or just taking slices where possible should be fine. I feel like especially in the python parsing subcrate there is a lot of cloning where it isn't actually necessary. Especially since I am vaguely planning to do things like the #41 that will require carrying around range information, so taking slices should help there.

One point of contention is that I want to keep the API sort of ergonomic and esp when hadleing python types you get these massive switch statements that end up doing pretty much the same, but you can't make generic without type inference crimes. Probably we'll have to come up with a more sucsinct type that carries just the information we want, but this will take some figuring out.

Hope this is enough info to get you started, but feel free to ask questions or brainstorm on topics if that would be helpful for you.

[savente93]

Oh and as a little addendum, because the project is pretty new the docs/guidelines in places can be quite sparse. Given that you know about Cow and Deref I'm guessing you're fairly knowledgeable in Rust already so you can probably figure things out, but if there are things that are unclear, either in technical documentation or things like contributing guidelines, I'd love it if you made issues (or PRs!) to let me know so we can improve them for the next users/contributors :)

[oopscompiled]

Might be nice to do this after we setup some benchmarks so we can see how much of a difference it makes.

Before we start refactoring for slices, I guess we should definitely set up something like DHAT, given that the main goal is to reduce owned copies. It will help us track the number of allocations and memcpy calls and I think it’s more precise for this case than just measuring execution time, as it directly points to where the cloning overhead is highest.

Here is more detailed review of profilers: https://nnethercote.github.io/perf-book/profiling.html

[savente93]

Setting up a benchmark is definitely something that's been on the todo list for a while. Personally I was more interested in either flamegraph or coz (though they are not mutually exclusive), but basically anything is better than nothing, so if you're interested in setting up DHAT, I wouldn't be opposed to that either. I think the benchmarks can use a lot of the same infrastructure, so it's a good thing to do regardless.

So if you're inerested in etting up a benchmark, feel free. I think the test site we use as an integration test should be a decent case to run the benches on.

[savente93]

little addemdum but was briefly poking around on the flamegraph repo when I saw this:

Lots of things that would seem like they would have terrible performance are actually cheap or free in Rust. Closures are fast. Initialization on the stack before moving to a Box is often compiled away. Clones are often compiled away. So, clone() away instead of fighting for too long to get the compiler to be happy about ownership!

Then make a flamegraph to see if any of that was actually expensive.

so who knows, it might not be a big deal after all. Setting up some kind of benchmark should definitely come first, just like you said

[savente93]

So I've setup a simple bench mark which produced this flamegraph:

I'm sure much about this setup can be improved (performance work is definitely not something I'm experienced in), so if you feel compelled to, feel free. I do think the flamegraph points to the fact that by far most of the time we are working on regex stuff (which means the reference expansion) which is not really that surprising, I knew that was a pretty inefficient way of doing things, but I also guess I didn't expect the effect to be this pronounced.

I guess my point is, you were right that clones are definitely not where we should be focusing our efforts to improve performance. I'll be leaving the ticket open for now, as it might be a good thing for someone who wants to gain more experience with the borrow checker, and probably can't hurt, but for now I'm not planing to work on this further. Also if you want to do something with DHAT or another profiler to gain more detailed insight, feel free as well!