Skip to content

Conversation

@iliana
Copy link
Contributor

@iliana iliana commented Oct 29, 2024

Not sure what changed between 1.81 and 1.82 but Clippy is once again complaining about InstanceManagerRequest::EnsureRegistered:

warning: large size difference between variants
   --> sled-agent/src/instance_manager.rs:335:1
    |
335 | /   enum InstanceManagerRequest {
336 | | /     EnsureRegistered {
337 | | |         propolis_id: PropolisUuid,
338 | | |         instance: InstanceEnsureBody,
339 | | |         // These are boxed because they are, apparently, quite large, and Clippy
...   | |
345 | | |         tx: oneshot::Sender<Result<SledVmmState, Error>>,
346 | | |     },
    | | |_____- the largest variant contains at least 448 bytes
...   |
351 | | /     EnsureState {
352 | | |         propolis_id: PropolisUuid,
353 | | |         target: VmmStateRequested,
354 | | |         tx: oneshot::Sender<Result<VmmPutStateResponse, Error>>,
355 | | |     },
    | | |_____- the second-largest variant contains at least 64 bytes
...   |
387 | |       },
388 | |   }
    | |___^ the entire enum is at least 0 bytes
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
    = note: `#[warn(clippy::large_enum_variant)]` on by default
help: consider boxing the large fields to reduce the total size of the enum
    |
338 |         instance: Box<InstanceEnsureBody>,
    |                   ~~~~~~~~~~~~~~~~~~~~~~~

Everything else is pretty minimal.

@iliana iliana requested review from hawkw and sunshowers October 29, 2024 17:49
@iliana iliana mentioned this pull request Oct 29, 2024
// others. Since we will generally send `EnsureRegistered` requests much
// less frequently than most of the others, boxing this seems like a
// reasonable choice...
instance: Box<InstanceEnsureBody>,
Copy link
Member

Choose a reason for hiding this comment

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

take it or leave it: perhaps, instead of boxing the InstanceEnsureBody and the SledIdentifiers separately, we should just take the whole variant and stick it inside one big box? Not performance critical, so this doesn't actually matter all that much, but it might feel a little nicer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will leave it, as to me it feels weird to box the entire variant.

Another option could be to do Box<(InstanceEnsureBody, SledIdentifiers)>. But I don't think it matters that much either way.

@iliana iliana enabled auto-merge (squash) October 29, 2024 21:13
@iliana iliana merged commit d4263cb into main Oct 29, 2024
17 checks passed
@iliana iliana deleted the iliana/rust-1.82 branch October 29, 2024 21:59
hawkw added a commit that referenced this pull request Nov 1, 2024
As of #6949, the `rust-toolchain.toml` file for Omicron uses Rust
1.82.0. This Rust release was not in nixpkgs at the last time the Nix
flake's lockfile was updated, so in order for Nix users to get the
correct toolchain, the flake must be updated. This commit includes the
results of running `nix flake update`, which updates the lockfile to a
Nixpkgs revision that includes the desired toolchain.
hawkw added a commit that referenced this pull request Nov 1, 2024
As of #6949, the `rust-toolchain.toml` file for Omicron uses Rust
1.82.0. This Rust release was not in nixpkgs at the last time the Nix
flake's lockfile was updated, so in order for Nix users to get the
correct toolchain, the flake must be updated. This commit includes the
results of running `nix flake update`, which updates the lockfile to a
Nixpkgs revision that includes the desired toolchain.
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.

3 participants