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 UstrSet for TypeInfo.properties_visited and TypeInfo.aliases #469

Conversation

kennethloeffler
Copy link
Member

@kennethloeffler kennethloeffler commented Oct 31, 2024

For better rbx_binary serialization performance, this PR changes the type of TypeInfo.properties_visited from HashSet<(Ustr, VariantType)> to UstrSet, and changes the type of TypeInfo.aliases from BTreeSet<Ustr> to UstrSet. These changes are sound because:

  • The second tuple field in TypeInfo.properties_visited entries is not used for anything outside of the collection, and it's impossible for an instance to contain multiple properties that have the same name, but different types.
  • The order of TypeInfo.aliases is unimportant.

On my machine, serialization benchmark results compared to latest master are as follows:

100 Folders/Serialize   time:   [20.219 µs 20.256 µs 20.298 µs]                                   
                        thrpt:  [37.165 MiB/s 37.242 MiB/s 37.309 MiB/s]
                 change:
                        time:   [-2.1144% -1.9337% -1.7269%] (p = 0.00 < 0.05)
                        thrpt:  [+1.7573% +1.9719% +2.1600%]
                        Performance has improved.

100 Deep Folders/Serialize                                                                             
                        time:   [22.818 µs 22.837 µs 22.858 µs]
                        thrpt:  [33.001 MiB/s 33.032 MiB/s 33.060 MiB/s]
                 change:
                        time:   [-2.5076% -2.3625% -2.2207%] (p = 0.00 < 0.05)
                        thrpt:  [+2.2711% +2.4196% +2.5721%]
                        Performance has improved.

100 100-line ModuleScripts/Serialize                                                                            
                        time:   [76.197 µs 76.256 µs 76.313 µs]
                        thrpt:  [48.338 MiB/s 48.374 MiB/s 48.412 MiB/s]
                 change:
                        time:   [+0.9836% +1.5364% +2.1002%] (p = 0.00 < 0.05)
                        thrpt:  [-2.0570% -1.5131% -0.9740%]
                        Change within noise threshold.

1,000 Parts/Serialize   time:   [1.3277 ms 1.3290 ms 1.3303 ms]                                   
                        thrpt:  [3.1464 MiB/s 3.1495 MiB/s 3.1526 MiB/s]
                 change:
                        time:   [-6.6982% -6.5511% -6.4099%] (p = 0.00 < 0.05)
                        thrpt:  [+6.8490% +7.0104% +7.1791%]
                        Performance has improved.

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 can't validate your performance claims because the benchmark results are honestly all over the place for me (the cost of being on Windows, I guess), but it doesn't seem to hurt anything so really all it can do is improve performance. LGTM.

@kennethloeffler
Copy link
Member Author

kennethloeffler commented Oct 31, 2024

To help reduce noise, it's generally good to have as few other things running on your system as possible. 1,000 parts is definitely more prone to problematic pollution than 10,000 parts too, so if I do any more performance work in the future, I'll make sure to add some chunkier (and more diverse) benchmarks

@kennethloeffler kennethloeffler merged commit 28b2fae into rojo-rbx:master Oct 31, 2024
3 checks passed
@kennethloeffler kennethloeffler deleted the binary-serializer-ustrmap-aliases-and-visisted branch October 31, 2024 16:56
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