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

Don't memoize description in process execute memoization #7916

Merged
merged 3 commits into from Jun 24, 2019

Conversation

Projects
None yet
4 participants
@gshuflin
Copy link
Contributor

commented Jun 20, 2019

#7745, we should revisit this solution when #7907 is decided upon

gshuflin added some commits Jun 20, 2019

Ignore description field on ExecuteProcessRequest
Ignore the description field for the PartialEq and Hash derivations
of ExecuteProcessRequest

@gshuflin gshuflin force-pushed the gshuflin:process_execute_memoization branch from c3bb7e0 to a104c46 Jun 21, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Looks great other than one place to deduplicate!

timeout: std::time::Duration::new(0, 0),
description: format!("Different message"),
jdk_home: None,
};

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 21, 2019

Contributor

In Rust, can you use a partial function to reduce the duplication? Like this:

const createProcessRequest = (description: str) => ExecuteProcessRequest([], BTree(), ..., description)

Easiest way to do this is probably an inner function like this https://stackoverflow.com/a/26685687.

Then your test can be:

fn execute_process_request_equality() {
  fn create_execute_process_request(description: str) {
    ...
  }

 let a = create_execute_process_request("One message");
 let b =  create_execute_process_request("Different message");
 
 assertions;
}

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 21, 2019

Contributor

The rust-ish way of this would be something like:

  let make_epr = |description: &'static str| ExecuteProcessRequest {
      argv: vec![],
      env: BTreeMap::new(),
      input_files: hashing::EMPTY_DIGEST,
      output_files: BTreeSet::new(),
      output_directories: BTreeSet::new(),
      timeout: std::time::Duration::new(0, 0),
      description: description.to_owned(),
      jdk_home: None,
    };

let a = make_epr("One message");
let b = make_epr("Different message");
@illicitonion
Copy link
Contributor

left a comment

Thanks for picking this up!

};
assert!(a == b);
let mut hasher = DefaultHasher::new();
assert!(a.hash(&mut hasher) == b.hash(&mut hasher));

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 21, 2019

Contributor

Unfortunately this isn't quite how the Hash API works; what you're actually comparing here is that () == ().

I think you need to do something like:

fn hash<Hashable: Hash>(hashable: &Hashable) -> u64 (
  let mut hasher = DefaultHasher::new();
  hashable.hash(&mut hasher);
  hasher.finish()
}

and then compare:

hash(&a) and hash(&b)

output_files: BTreeSet::new(),
output_directories: BTreeSet::new(),
timeout: std::time::Duration::new(0, 0),
description: format!("One message"),

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 21, 2019

Contributor

Clippy is probably going to complain at you for using format!(&'static str) because it's significantly slower than any of:
"One message".to_string(), "One message".to_owned(), or String::from("One message")

In a test it doesn't matter so much, but it's probably a good habit to get into :)

timeout: std::time::Duration::new(0, 0),
description: format!("Different message"),
jdk_home: None,
};

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 21, 2019

Contributor

The rust-ish way of this would be something like:

  let make_epr = |description: &'static str| ExecuteProcessRequest {
      argv: vec![],
      env: BTreeMap::new(),
      input_files: hashing::EMPTY_DIGEST,
      output_files: BTreeSet::new(),
      output_directories: BTreeSet::new(),
      timeout: std::time::Duration::new(0, 0),
      description: description.to_owned(),
      jdk_home: None,
    };

let a = make_epr("One message");
let b = make_epr("Different message");
use std::hash::Hash;

#[test]
fn execute_process_request_equality() {

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 21, 2019

Contributor

It's probably worth also adding a test which shows that if something other than description changes, the requests are equal and do hash the same, as we're messing with that generality.

@gshuflin gshuflin force-pushed the gshuflin:process_execute_memoization branch from a104c46 to a2c2712 Jun 21, 2019

@illicitonion illicitonion merged commit b396810 into pantsbuild:master Jun 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.