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

Change type of InstanceBuilder.properties to Vec<(Ustr, Variant)> #467

Conversation

kennethloeffler
Copy link
Member

This PR changes the type of InstanceBuilder.properties from UstrMap<Variant to Vec<(Ustr, Variant)> because I noticed that we were spending a significant amount of time just inserting into this HashMap during deserialization, and we get no benefit from this being a HashMap. The number of keys is usually small, and we only do a lookup here:

fn add_property(instance: &mut Instance, canonical_property: &CanonicalProperty, value: Variant) {
if let Some(PropertySerialization::Migrate(migration)) = canonical_property.migration {
let new_property_name = &migration.new_property_name;
let old_property_name = canonical_property.name;
if !instance.builder.has_property(new_property_name) {
log::trace!(
"Attempting to migrate property {old_property_name} to {new_property_name}"
);

which is a very cold path.

On my machine, this improves rbx_binary's "Deserialize 10,000 Parts" benchmark by ~11%, and will be even more significant on larger workloads.

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

I was skeptical of this one because it turns has_property into an O(n) operation rather than (theoretically) O(1). However, I just did a benchmark and on my machine the worst-case scenario of having all 3691 properties took 47200 nanoseconds... So I think we're probably fine.

@kennethloeffler
Copy link
Member Author

kennethloeffler commented Oct 30, 2024

RIght, linear time property lookups on an InstanceBuilder would only be bad if it were an incredibly common operation - but it isn't, both for us and for our consumers. In this case, the cost of hash table bookkeeping outweighs the benefit of constant time lookup.

In contrast, attempting this kind of optimization on Instance.properties would actually make things worse, since the rbx_binary serializer has to look up lots of random properties over and over again, and consumers often want to do the same thing

@kennethloeffler kennethloeffler merged commit 9cbfcd5 into rojo-rbx:master Oct 30, 2024
3 checks passed
@kennethloeffler kennethloeffler deleted the instance-builder-vec-properties branch October 30, 2024 20:12
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