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

Use ahash for most HashMaps and HashSets #466

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

kennethloeffler
Copy link
Member

This PR replaces most HashMaps with AHashMap. I did it the lazy way with type aliases, didn't do anything to rbx_xml (yet?), and haven't touched the changelogs yet, because I am tired and I want to go to bed! It's kind of invasive, and might make the library harder to use, but we do get a nice performance boost from it.

On my machine, this improves rbx_binary's "Deserialize 10,000 Parts" and "Serialize 10,000 Parts" benchmarks by ~15% and ~19%, respectively.

@kennethloeffler kennethloeffler marked this pull request as draft October 30, 2024 00:34
@Dekkonot
Copy link
Member

It's kind of invasive, and might make the library harder to use, but we do get a nice performance boost from it.

I do have some concerns about changing the reflection database to use AHashMap, since we aren't the only consumers. The only other public interface this would impact is WeakDom::into_raw though and I think it's probably fine to leak that. I don't know how many people are using that API but it's probably fine to slightly break it since we're already making a breaking change anyway.

This will impact Argon and Rojo, but those are the only ones I can think of off the top of my head. They'll already have to do a bunch of changes to update to rbx_weak_dom 3.0 though, so it's probably fine.

@kennethloeffler
Copy link
Member Author

After some further benchmarking, using ahash in rbx_reflection is a bit of a wash. It didn't effect rbx_binary's deserialization performance at all, and if anything made rbx_binary's serialization performance worse. Good for me, fewer changelogs to touch :D

@kennethloeffler kennethloeffler marked this pull request as ready for review October 30, 2024 21:23
@kennethloeffler kennethloeffler changed the title Use AHashMap in place of most HashMaps Use ahash variants for most HashMaps and HashSets Oct 30, 2024
@kennethloeffler kennethloeffler changed the title Use ahash variants for most HashMaps and HashSets Use ahash for most HashMaps and HashSets Oct 30, 2024
rbx_dom_weak/CHANGELOG.md Outdated Show resolved Hide resolved
@kennethloeffler kennethloeffler merged commit 72e772b into rojo-rbx:master Oct 31, 2024
3 checks passed
@kennethloeffler kennethloeffler deleted the ahash branch October 31, 2024 05:43
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.

2 participants