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

debug execution: implement actor redirects in engine #605

Merged
merged 2 commits into from
Jun 21, 2022

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Jun 2, 2022

This seems like the simplest way to do it, without getting into a mess trying to implement intercepting or generic engines.

@vyzo vyzo requested review from Stebalien and raulk June 2, 2022 20:35
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

This seems like a very reasonable way to do this.

Comment on lines +191 to +195
.as_ref()
.map(|v| v.iter().cloned().collect());
Copy link
Member

Choose a reason for hiding this comment

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

If we're not overriding anything, we might as well just have an empty hashmap.

Suggested change
.as_ref()
.map(|v| v.iter().cloned().collect());
.as_ref()
.unwrap_or_default().iter().cloned().collect();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, do you want to drop the option from the data structure?
I thought it would be faster to None check instead of a get in an empty map, but if there is no difference we might as well.

Copy link
Member

Choose a reason for hiding this comment

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

An option prevents us from hashing on access when there are no redirects set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so lets keep it and decline steb's suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

An option prevents us from hashing on access when there are no redirects set.

Rust is smarter than that https://github.com/rust-lang/hashbrown/blob/f3a242fb48e73df704d27c9722e30ebda5dd8e4f/src/map.rs#L1099-L1104

@@ -223,8 +234,19 @@ impl Engine {
Ok(())
}

fn with_redirect<'a>(&'a self, k: &'a Cid) -> &'a Cid {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this explicit lifetime?

Copy link
Contributor Author

@vyzo vyzo Jun 2, 2022

Choose a reason for hiding this comment

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

yes, i refuses to compile without it.

@raulk
Copy link
Member

raulk commented Jun 2, 2022

This was easier and cleaner than I expected, so no need for generics. Or wrapping the engine. KISS.

@vyzo vyzo force-pushed the feat/debug-execution branch from 6693eba to 45a9e9e Compare June 10, 2022 17:21
@vyzo
Copy link
Contributor Author

vyzo commented Jun 10, 2022

rebased on master for the logging fix.

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #605 (29ac9a3) into master (29ac9a3) will not change coverage.
The diff coverage is n/a.

❗ Current head 29ac9a3 differs from pull request most recent head 45a9e9e. Consider uploading reports for the commit 45a9e9e to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #605   +/-   ##
=======================================
  Coverage   51.39%   51.39%           
=======================================
  Files         120      120           
  Lines        9512     9512           
=======================================
  Hits         4889     4889           
  Misses       4623     4623           

@vyzo vyzo marked this pull request as ready for review June 10, 2022 17:42
@@ -116,6 +116,9 @@ pub struct NetworkConfig {
///
/// DEFAULT: The price-list for the current network version.
pub price_list: &'static PriceList,

/// Actor redirects for debug execution
pub actor_redirect: Option<Vec<(Cid, Cid)>>,
Copy link
Member

Choose a reason for hiding this comment

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

No reason to make this an option either. An empty vector is free.

@raulk
Copy link
Member

raulk commented Jun 21, 2022

Now that master is M2, I will merge this to release/v1 and forward port from there.

@raulk raulk changed the base branch from master to release/v1 June 21, 2022 20:26
@raulk raulk merged commit c05db59 into release/v1 Jun 21, 2022
@raulk raulk deleted the feat/debug-execution branch June 21, 2022 20:26
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.

None yet

4 participants