Skip to content

Conversation

@krakow10
Copy link
Contributor

@krakow10 krakow10 commented Jun 1, 2025

This makes the interleaved bytes readers and writers work directly with iterators. Many intermediate allocations and copies are omitted, and much of the serialization and deserialization code dealing with interleaved bytes is greatly erginomified. I'm shocked that this wasn't a decent performance uplift, there must be something weird going on. Benchmarks show +6% serialize and +2% deserialize, except the unreliable benchmark which is still random.

As for the unsafe code, I included an equivalent but theoretically slower safe implementation in a comment next to it. The safe implementation benchmarks -1% slower which is within noise. I opted to just use the safe version because the performance impact is negligible.

More effort could be put into avoiding intermediate allocations in the Content serialize, deserialize, and text_deserializer. If type mismatch errors are dropped, many more optimizations could be made to avoid intermediate allocations.

Note: The write_interleaved_bytes_* functions are implemented onto ChunkBuilder rather than RbxWriteExt to be able to directly take a &mut [u8] from the output buffer.

Edit: Not sure why this is failing to compile on the CI. It builds fine and passes all tests on rustc 1.87. Looks like it has to do with the syntax in 3679993

@Dekkonot
Copy link
Member

Dekkonot commented Jun 9, 2025

Edit: Not sure why this is failing to compile on the CI. It builds fine and passes all tests on rustc 1.87. Looks like it has to do with the syntax in 3679993

You're using Nightly. That might explain your performance woes too.

@krakow10
Copy link
Contributor Author

krakow10 commented Jun 10, 2025

I'm very certain I'm not using nightly.

Evidence
quat@quat-desktop ~ [127]> rustc --version
rustc 1.87.0 (17067e9ac 2025-05-09) (Arch Linux rust 1:1.87.0-2)
fn main(){
	dbg!(rustc_version::version_meta());
}
[src/main.rs:2:2] rustc_version::version_meta() = Ok(
    VersionMeta {
        semver: Version {
            major: 1,
            minor: 87,
            patch: 0,
        },
        commit_hash: Some(
            "17067e9ac6d7ecb70e50f92c1944e545188d2359",
        ),
        commit_date: Some(
            "2025-05-09",
        ),
        build_date: None,
        channel: Stable,
        host: "x86_64-unknown-linux-gnu",
        short_version_string: "rustc 1.87.0 (17067e9ac 2025-05-09) (Arch Linux rust 1:1.87.0-2)",
        llvm_version: Some(
            LlvmVersion {
                major: 20,
                minor: 1,
            },
        ),
    },
)

Installing 1.86 with rustup and building with that shows the same errors as the CI, so it must be something brand new in 1.87. One way to express this in earlier versions would be to spell out the exact type of the returned iterator.

@Dekkonot
Copy link
Member

Hmm. You'll have to forgive me, I was out of date on my end as well and the error I got matched the CI error + it said it was an unstable feature so I just assumed.

It does appear as though this was stabilized in 1.87.0 and for some reason the version we're downloading in CI is 1.86.0, despite us not pinning a version at all. Unsure what to do about that if anything. Maybe we simply wait patiently for GitHub to update their runners.

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