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

geoprobe: simple geolocation probing refactoring #510

Merged
merged 10 commits into from
Oct 8, 2021

Conversation

Soptq
Copy link
Contributor

@Soptq Soptq commented Sep 29, 2021

More info in #498 .

standalone/pruntime/Readme.md Outdated Show resolved Hide resolved
crates/phactory/src/system/side_tasks/geo_probe.rs Outdated Show resolved Hide resolved
.derive_ecdh_key()
.expect("Should never failed with valid identity key; qed.");
// TODO: currently assume contract key equals to local ecdh key
let public_contract_ecdh_key = my_ecdh_key.clone().public();
Copy link
Contributor

Choose a reason for hiding this comment

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

@shelvenzhou I think this one should be the target contract ecdh pubkey.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the last remaining problem

UnknownError,
}

impl fmt::Display for GeoProbeError {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kvinwang I guess there are easier ways to write Errors?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there are easier ways to write Errors?

There is a crate derive_more to #[derive(Display)] for no_std and a thiserror for std.

Copy link
Contributor

@h4x3rotab h4x3rotab Sep 30, 2021

Choose a reason for hiding this comment

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

We already have anyhow as pruntime is like a binary than a library. What do you suggest?

crates/phactory/src/contracts/geolocation.rs Show resolved Hide resolved
@h4x3rotab h4x3rotab changed the base branch from master to para-1 September 29, 2021 19:33
@h4x3rotab h4x3rotab changed the base branch from para-1 to master September 29, 2021 19:33
@kvinwang
Copy link
Collaborator

kvinwang commented Sep 30, 2021

We have added some memory statistics in the get_info on the master.
Could you please compare the memory usage between with and without geolocation enabled?

You can get the mem stat with this command:

$ curl http://localhost:8000/get_info | jq -r '.payload | fromjson | .memory_usage'
{
  "rust_peak_used": 174590146,
  "rust_used": 51173836,
  "total_peak_used": 278560768
}

Copy link
Contributor

@h4x3rotab h4x3rotab left a comment

Choose a reason for hiding this comment

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

Almost LGTM. Only left the ECDH key problem. I will chat with @shelvenzhou offline about it.

@h4x3rotab
Copy link
Contributor

Btw, if you want to update the proto file reference, you just need to cd to the proto directory, use git checkout to locate it to the correct branch / commit, then return back to the main repo and run git add proto/.

@Soptq
Copy link
Contributor Author

Soptq commented Sep 30, 2021

We have added some memory statistics in the get_info on the master. Could you please compare the memory usage between with and without geolocation enabled?

You can get the mem stat with this command:

$ curl http://localhost:8000/get_info | jq -r '.payload | fromjson | .memory_usage'
{
  "rust_peak_used": 174590146,
  "rust_used": 51173836,
  "total_peak_used": 278560768
}

With geolocation enabled:

MemoryUsage {
    rust_used: 1180828,
    rust_peak_used: 74689956,
    total_peak_used: 115478528,
}

Without geolocation enabled::

MemoryUsage {
    rust_used: 1034493,
    rust_peak_used: 3656772,
    total_peak_used: 42037248,
}

Both memory statistics are sampled at the end of the e2e test.

@h4x3rotab
Copy link
Contributor

LGTM. We are ready to merge. The feature not enabled by default, right?

@Soptq
Copy link
Contributor Author

Soptq commented Oct 2, 2021

LGTM. We are ready to merge. The feature not enabled by default, right?

Yes, disabled by default.

@h4x3rotab h4x3rotab merged commit cf24eb8 into Phala-Network:master Oct 8, 2021
@Soptq Soptq deleted the dev-geo-probing branch October 10, 2021 05:27
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

3 participants