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

Make the sizes of the members of `enum Node` more uniform #6545

Merged
merged 2 commits into from Sep 24, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented Sep 23, 2018

Problem

The clippy shard is currently failing with a notice that there is a large variance between the sizes of the members of the Node enum. It refers to https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#large_enum_variant

According to mem::size_of, the sizes are currently:

  DigestFile(DigestFile),              // 32
  ExecuteProcess(ExecuteProcess),      // 200
  ReadLink(ReadLink),                  // 24
  Scandir(Scandir),                    // 24
  Select(Select),                      // 232
  Snapshot(Snapshot),                  // 16
  Task(Task),                          // 192

Solution

Box the three largest members to appease clippy.

Result

No appreciable difference in performance; happy clippy.

@stuhood stuhood changed the title WIP: Make the sizes of the members of `enum Node` more uniform Make the sizes of the members of `enum Node` more uniform Sep 23, 2018

@stuhood stuhood requested a review from illicitonion Sep 23, 2018

@stuhood stuhood force-pushed the twitter:stuhood/clippy-in-a-box branch from c07ab1a to 19e5ba5 Sep 23, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Works for me, thanks :) Probably significantly decreases memory consumption...

If you're just doing this to appease clippy, and would rather disable the lint, I'd be fine with that too, but I suspect the memory savings will actually be non-trivial.

@@ -841,12 +841,12 @@ impl NodeTracer<NodeKey> for Tracer {
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub enum NodeKey {
DigestFile(DigestFile),
ExecuteProcess(ExecuteProcess),
ExecuteProcess(Box<ExecuteProcess>),

This comment has been minimized.

@illicitonion

illicitonion Sep 24, 2018

Contributor

May be worth adding a comment on the enum about why some things are boxed

@stuhood stuhood merged commit 1924650 into pantsbuild:master Sep 24, 2018

@stuhood stuhood deleted the twitter:stuhood/clippy-in-a-box branch Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment