Skip to content

Conversation

@krakow10
Copy link
Contributor

@krakow10 krakow10 commented May 30, 2025

I identified a performance issue observed in #533, and found that it is applicable to the serializer in general. Basically the part where it scans instance properties for aliases in serialize_properties can be omitted.

According to the "Miner's Haven/Serialize" benchmark, this improves serializer throughput by +37%. Deserialization is not touched and is within noise, ±3%.

The core concept behind this change is to collect references to property values in collect_type_info, and then use those references in serialize_properties instead of having to scan the instances for the properties again. The code organization can probably be substantially improved, but I don't have any ideas at the moment so please do advise me. The giant function signatures are not nice in serialize_properties (clippy is NOT happy about that) and the control flow is particularly twisted there too. I've squeaked under the 7 argument limit, but there are still simplifications to be made. The problem is mostly the type_mismatch and invalid_value closures making the function signatures huge.

There are four previous incarnations of this at various stages because there is truly a lot of conflicting requirements to be met for everything to work correctly... Notably migration is at odds with taking Variant inner type references, and interleaved bytes is at odds with building prop chunks procedurally. I wouldn't have been able to pull this off without the excellent testing suite.

@krakow10 krakow10 force-pushed the propinfo5 branch 2 times, most recently from 1fbd57d to 5aa5c43 Compare May 31, 2025 22:36
@krakow10 krakow10 changed the title rbx_binary: serializer: Index Properties Once rbx_binary: Index Properties Once (+37%) Jun 1, 2025
@krakow10
Copy link
Contributor Author

krakow10 commented Jun 10, 2025

I've found a test case that's not covered! No test checks to see whether non-serializing properties actually do not serialize.

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.

1 participant