Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

underfin
Copy link
Contributor

@underfin underfin commented Mar 15, 2024

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
    }

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, though this is a breaking change :-(

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

@underfin underfin force-pushed the perf-avoid-extra-alloc branch from 6c08c44 to 89244e8 Compare March 15, 2024 10:35
@underfin
Copy link
Contributor Author

underfin commented Mar 15, 2024

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
Copy link
Member

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
Copy link
Contributor Author

OK. Get it.

@loewenheim
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

underfin commented Mar 19, 2024

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

@Swatinem
Copy link
Member

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.

@Swatinem Swatinem merged commit f80bd6c into getsentry:master Mar 19, 2024
4 checks passed
@underfin
Copy link
Contributor Author

underfin commented Mar 20, 2024

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

@underfin
Copy link
Contributor Author

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

@Swatinem
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants