getsentry / rust-sourcemap

A library for rust that implements basic sourcemap handling
Other
224 stars 28 forks source link

perf: using Arc<str> instead of String #84

Closed underfin closed 8 months ago

underfin commented 8 months ago

Here has some unnecessary memory alloc, and the pr intends to fix them.

image

Here has a example.

fn add_source_with_id(&mut self, src: &str, old_id: u32) -> u32 {
        let count = self.sources.len() as u32;
        let id = *self.source_map.entry(src.into()).or_insert(count); // Here `src.into` -> `String` only calculate hash, it is unnecessary.
        if id == count {
            self.sources.push(src.into()); // Here `src.into` -> `String` to store,  it is unnecessary.
            self.sources_mapping.push(old_id);
        }
        id
    }
underfin commented 8 months ago

In a bunch of places, I was thinking about using IndexSet instead of manually implementing that same functionality as well.

I'm not sure the performance using IndexSet, if you refactor it and I'm ok to test it and give performance profile for it.

Swatinem commented 8 months ago

Not sure about the performance aspect of it. I just wanted to clean up the code, because implementing this manually looks out of place. Though I expect IndexSet to have better performance rather than implementing it manually with a Vec + HashMap.

underfin commented 8 months ago

OK. Get it.

loewenheim commented 8 months ago

IndexSet/IndexMap have the minor flaw that you can't change a set element (or the key of a map element, respectively) in-place. If you want to modify the element at index i this becomes a whole lot more annoying. But honestly, I'm inclined to nix at least set_source entirely. In strip_prefixes there's unfortunately no way around copying the entire set of sources.

underfin commented 8 months ago

Maybe we can merge it and refactor it at next. Though it has a break change, but the performance it is important for oxc/swc etc.

underfin commented 8 months ago

@Swatinem What can I do for merge the pr? Or Could you publish an alpha/beta for this? Thank you.

Swatinem commented 8 months ago

Could you publish an alpha/beta for this?

That would be the more difficult thing I believe :-D

Sentry has some custom automation around publishing to crates.io, and I’m not quite sure we can even do prereleases.

I can merge this PR as is, though I would also like to get #77 in, as that also has some breaking changes, to minimize the number of major releases. Unfortunately, we don’t really have enough time to properly maintain this crate at the time :-( There is a lot of cleanups we would love to do as well.

underfin commented 8 months ago

Thank you. @Swatinem Could you publish an new version for them?

underfin commented 8 months ago

@Swatinem Sorry for ping again. Maybe you missing my response.

Swatinem commented 8 months ago

Published 8.0.0 with this breaking change, and another breaking change due to the range mapping proposal.

Not too happy about not being able to maintain API stability for a longer time frame, but I just have to face reality here :-D