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

Feature/serde feature #5149

Merged
merged 18 commits into from
Jan 28, 2024
Merged

Feature/serde feature #5149

merged 18 commits into from
Jan 28, 2024

Conversation

KirmesBude
Copy link
Contributor

@KirmesBude KirmesBude commented Jan 27, 2024

Connections
#3609

Description
Allows using feature serde on wgpu, wgpu-core and wgpu-types.
wgpu-types no longer has trace and replay features, but an implicit serde feature.
wgpu-core has an explicit serde feature and removes the serial-pass feature.
wgpu now has an explicit serde feature.

Almost all occurrences of cfg_attr(feature = "trace") and cfg_attr(feature = "replay") have been replaced with cfg_attr(feature = "serialize") and cfg_attr(feature = "deserialize") respectively.
For wgpu and wgpu-core the usage for trace and replay should not have changed. If you are using serial-pass you should use serde instead. For wgpu-types if you have previously enabled the trace or replay feature you need to migrate to serde.

Testing
cargo test --package <package> --features <feature>
for the following packages: wgpu-types, wgpu-core, wgpu
for the following features: "None", serde, (trace, replay)[if available] [*no combinations]

Compile timings
Comparing 273684f with f748c43 on debug.
With default features takes about 25 sec.
With serde takes around the same time (38 sec).
With deserialize (on first commit) takes about the same amount of time (38 sec).
With serialize takes about 27 sec.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten (fails somewhere in winit)
  • Run cargo xtask test to run tests. (yes, and it fails in some places, no sure what to do with the results)
  • Add change to CHANGELOG.md. See simple instructions inside file.

@KirmesBude KirmesBude requested a review from a team as a code owner January 27, 2024 14:29
@Wumpf Wumpf self-requested a review January 27, 2024 16:43
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry, fairly important to highlight this feature config change :)

Handling of serial-pass looks broken to me but I'm a bit on the fence whether we still need this one 🤔.
I'm actually overall here with what @cwfitzgerald's comment on the issue and would suggest joining all of serde, serial-pass, serialize and deserialize to one feature, I doubt anyone needs this level of granularity realistically.

Anyways, thanks so much for looking into this! Happy to merge once the serial-pass story, docs and changelog is sorted out! :)

wgpu/Cargo.toml Outdated Show resolved Hide resolved
wgpu-core/Cargo.toml Outdated Show resolved Hide resolved
wgpu-core/Cargo.toml Outdated Show resolved Hide resolved
wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
@KirmesBude
Copy link
Contributor Author

Please add a changelog entry, fairly important to highlight this feature config change :)

Handling of serial-pass looks broken to me but I'm a bit on the fence whether we still need this one 🤔. I'm actually overall here with what @cwfitzgerald's comment on the issue and would suggest joining all of serde, serial-pass, serialize and deserialize to one feature, I doubt anyone needs this level of granularity realistically.

Anyways, thanks so much for looking into this! Happy to merge once the serial-pass story, docs and changelog is sorted out! :)

Thanks for the heads up about serial-pass. Should be fixed now, I hope.
Wasn't sure where to put stuff for the Changelog. Let me know if the current version works for you.

@KirmesBude KirmesBude requested a review from Wumpf January 27, 2024 19:47
@KirmesBude
Copy link
Contributor Author

Oh and about merging everything to serde. I am also fine with that. Just let me know and I will try to do the necessary changes.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

thanks for the fixes! changelog looks great
Yeah if you don't mind changing it again right away I'd say we should go with just serde, it makes all the cfgs so much simpler and reduces the number of feature flags and thus possible permutations by a lot.

wgpu/Cargo.toml Outdated Show resolved Hide resolved
@KirmesBude
Copy link
Contributor Author

KirmesBude commented Jan 28, 2024

thanks for the fixes! changelog looks great Yeah if you don't mind changing it again right away I'd say we should go with just serde, it makes all the cfgs so much simpler and reduces the number of feature flags and thus possible permutations by a lot.

Yeah, I will try that. On wgpu-types level it is trivial, but I am not sure how to approach it for wgpu-core and wgpu.
serial-pass is separate but should be part of serde. For that reason trace/replaycan not imply serde and I can not use serde in the source code.

Edit: I could replace feature = "serialize" with any(feature = "trace", feature = "serde") and feature = "deserialize" with any(feature = "replay", feature = "serde"). Is that what you had in mind?

@Wumpf
Copy link
Member

Wumpf commented Jan 28, 2024

Edit: I could replace feature = "serialize" with any(feature = "trace", feature = "serde") and feature = "deserialize" with any(feature = "replay", feature = "serde"). Is that what you had in mind?

I was thinking simpler still that both replay and trace would just imply the serde feature and all checks where we decide whether a thing implements the serde serialization/deserialization would only check for the serde feature

@Wumpf
Copy link
Member

Wumpf commented Jan 28, 2024

fyi I just posted about this also on the wgpu Matrix chat room just in case anyone has a good reason that we're not seeing right now on why deserialize & serialize should stay separate :)
https://matrix.to/#/!FZyQrssSlHEZqrYcOb:matrix.org/$DnwtH2xxPKoTq7OjVYEVoyUHzZMR3mVRIAvOhy3_GeI?via=matrix.org&via=mozilla.org&via=envs.net

@KirmesBude
Copy link
Contributor Author

KirmesBude commented Jan 28, 2024

Edit: I could replace feature = "serialize" with any(feature = "trace", feature = "serde") and feature = "deserialize" with any(feature = "replay", feature = "serde"). Is that what you had in mind?

I was thinking simpler still that both replay and trace would just imply the serde feature and all checks where we decide whether a thing implements the serde serialization/deserialization would only check for the serde feature

With your earlier comment about serial-pass this means that trace/replay implies serde which implies serial-pass. So trace/replay implies serial-pass. Is that fine?

@Wumpf
Copy link
Member

Wumpf commented Jan 28, 2024

Ah apologies for the confusion! If we go with serde for both serialize & deserialize then serial-pass should also go away I figure. Essentially we'd bulldozer everything over to only have serde, trace->serde, replay->serde.
Apparently, the original concern was compile time performance #619 (comment) but it looks like we don't have numbers for this and the setup with just serde implying both serialization and deserialization is much more common so I'll happily take all the blame (:

@KirmesBude KirmesBude requested a review from Wumpf January 28, 2024 18:09
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

awesome, yeah this is much cleaner! Will merge after changelog hickup is resolved!

CHANGELOG.md Outdated Show resolved Hide resolved
wgpu-core/src/command/compute.rs Outdated Show resolved Hide resolved
wgpu/Cargo.toml Outdated Show resolved Hide resolved
@Wumpf Wumpf enabled auto-merge (squash) January 28, 2024 19:44
@Wumpf Wumpf linked an issue Jan 28, 2024 that may be closed by this pull request
@Wumpf Wumpf merged commit 4face1c into gfx-rs:trunk Jan 28, 2024
27 checks passed
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.

wgpu-types error when using serde feature without tracing feature
2 participants