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

feat(rust): avoiding memory fragmentation by reducing allocations #8640

Merged
merged 3 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/rust/get_started/examples/bob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ impl Worker for Echoer {
println!("\n[✓] Address: {}, Received: {:?}", ctx.address(), msg);

// Echo the message body back on its return_route.
ctx.send(msg.return_route(), msg.into_body()?).await
ctx.send(msg.return_route().clone(), msg.into_body()?).await
}
}

Expand Down
2 changes: 1 addition & 1 deletion examples/rust/get_started/src/echoer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ impl Worker for Echoer {
println!("Address: {}, Received: {:?}", ctx.address(), msg);

// Echo the message body back on its return_route.
ctx.send(msg.return_route(), msg.into_body()?).await
ctx.send(msg.return_route().clone(), msg.into_body()?).await
}
}
2 changes: 1 addition & 1 deletion examples/rust/get_started/src/relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ impl Worker for Relay {
local_message = local_message.pop_front_onward_route()?;
local_message = local_message.prepend_front_onward_route(&self.route); // Prepend predefined route to the onward_route

let prev_hop = local_message.return_route_ref().next()?.clone();
let prev_hop = local_message.return_route().next()?.clone();

if let Some(info) = ctx
.flow_controls()
Expand Down
4 changes: 2 additions & 2 deletions implementations/rust/ockam/ockam/src/relay_service/relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl Worker for Relay {
.await?;

// Remove the last hop so that just route to the node itself is left
self.forward_route.modify().pop_back();
self.forward_route = self.forward_route.clone().modify().pop_back().into();

Ok(())
}
Expand All @@ -88,7 +88,7 @@ impl Worker for Relay {
.prepend_front_onward_route(&self.forward_route);

let next_hop = local_message.next_on_onward_route()?;
let prev_hop = local_message.return_route_ref().next()?;
let prev_hop = local_message.return_route().next()?;

if let Some(info) = ctx
.flow_controls()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ impl Worker for RelayService {
let secure_channel_local_info =
SecureChannelLocalInfo::find_info(message.local_message()).ok();

let forward_route = message.return_route();
let forward_route = message.return_route().clone();
let requested_relay_address = message.into_body()?;

let requested_relay_name = if requested_relay_address == "register" {
Expand Down
11 changes: 7 additions & 4 deletions implementations/rust/ockam/ockam/src/remote/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ impl Worker for RemoteRelay {
msg: Routed<Self::Message>,
) -> Result<()> {
if msg.msg_addr() == self.addresses.main_remote {
let return_route = msg.return_route();
let mut local_message = msg.into_local_message();

// Remove my address from the onward_route
Expand All @@ -53,8 +52,12 @@ impl Worker for RemoteRelay {
}

if !self.completion_msg_sent {
info!(registration_route = %self.registration_route, "RemoteRelay registered with route: {}", return_route);
let address = match return_route.recipient()?.to_string().strip_prefix("0#")
info!(registration_route = %self.registration_route, "RemoteRelay registered with route: {}", local_message.return_route);
let address = match local_message
.return_route
.recipient()?
.to_string()
.strip_prefix("0#")
{
Some(addr) => addr.to_string(),
None => return Err(OckamError::InvalidResponseFromRelayService)?,
Expand All @@ -63,7 +66,7 @@ impl Worker for RemoteRelay {
ctx.send_from_address(
self.addresses.completion_callback.clone(),
RemoteRelayInfo::new(
return_route,
local_message.return_route,
address,
self.addresses.main_remote.clone(),
self.flow_control_id.clone(),
Expand Down
6 changes: 3 additions & 3 deletions implementations/rust/ockam/ockam_api/src/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,19 @@ pub fn extract_address_value(input: &str) -> Result<String, ApiError> {
Node::CODE => {
addr = p
.cast::<Node>()
.ok_or(ApiError::message("Failed to parse `node` protocol"))?
.ok_or_else(|| ApiError::message("Failed to parse `node` protocol"))?
davide-baldo marked this conversation as resolved.
Show resolved Hide resolved
.to_string();
}
Service::CODE => {
addr = p
.cast::<Service>()
.ok_or(ApiError::message("Failed to parse `service` protocol"))?
.ok_or_else(|| ApiError::message("Failed to parse `service` protocol"))?
.to_string();
}
Project::CODE => {
addr = p
.cast::<Project>()
.ok_or(ApiError::message("Failed to parse `project` protocol"))?
.ok_or_else(|| ApiError::message("Failed to parse `project` protocol"))?
.to_string();
}
code => return Err(ApiError::message(format!("Protocol {code} not supported"))),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ impl Worker for CredentialIssuerWorker {
Ok(secure_channel_info) => secure_channel_info,
Err(_e) => {
let resp = Response::bad_request_no_request("secure channel required").to_vec()?;
c.send(m.return_route(), resp).await?;
c.send(m.return_route().clone(), resp).await?;
return Ok(());
}
};

let from = Identifier::from(secure_channel_info.their_identifier());
let return_route = m.return_route();
let return_route = m.return_route().clone();
let body = m.into_body()?;
let mut dec = Decoder::new(&body);
let req: RequestHeader = dec.decode()?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ impl Worker for DirectAuthenticatorWorker {
Ok(secure_channel_info) => secure_channel_info,
Err(_e) => {
let resp = Response::bad_request_no_request("secure channel required").to_vec()?;
c.send(m.return_route(), resp).await?;
c.send(m.return_route().clone(), resp).await?;
return Ok(());
}
};

let from = Identifier::from(secure_channel_info.their_identifier());
let return_route = m.return_route();
let return_route = m.return_route().clone();
let body = m.into_body()?;
let mut dec = Decoder::new(&body);
let req: RequestHeader = dec.decode()?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ impl Worker for EnrollmentTokenAcceptorWorker {
Ok(secure_channel_info) => secure_channel_info,
Err(_e) => {
let resp = Response::bad_request_no_request("secure channel required").to_vec()?;
c.send(m.return_route(), resp).await?;
c.send(m.return_route().clone(), resp).await?;
return Ok(());
}
};

let from = Identifier::from(secure_channel_info.their_identifier());
let return_route = m.return_route();
let return_route = m.return_route().clone();
let body = m.into_body()?;
let mut dec = Decoder::new(&body);
let req: RequestHeader = dec.decode()?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ impl Worker for EnrollmentTokenIssuerWorker {
Ok(secure_channel_info) => secure_channel_info,
Err(_e) => {
let resp = Response::bad_request_no_request("secure channel required").to_vec()?;
c.send(m.return_route(), resp).await?;
c.send(m.return_route().clone(), resp).await?;
return Ok(());
}
};

let from = Identifier::from(secure_channel_info.their_identifier());
let return_route = m.return_route();
let return_route = m.return_route().clone();
let body = m.into_body()?;
let mut dec = Decoder::new(&body);
let req: RequestHeader = dec.decode()?;
Expand Down
21 changes: 11 additions & 10 deletions implementations/rust/ockam/ockam_api/src/cli_state/cli_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,15 +186,16 @@ impl CliState {
/// Returns the default backup directory for the CLI state.
pub fn backup_default_dir() -> Result<PathBuf> {
let dir = Self::default_dir()?;
let dir_name =
dir.file_name()
.and_then(|n| n.to_str())
.ok_or(CliStateError::InvalidOperation(
"The $OCKAM_HOME directory does not have a valid name".to_string(),
))?;
let parent = dir.parent().ok_or(CliStateError::InvalidOperation(
"The $OCKAM_HOME directory does not a valid parent directory".to_string(),
))?;
let dir_name = dir.file_name().and_then(|n| n.to_str()).ok_or_else(|| {
CliStateError::InvalidOperation(
"The $OCKAM_HOME directory does not have a valid name".to_string(),
)
})?;
let parent = dir.parent().ok_or_else(|| {
CliStateError::InvalidOperation(
"The $OCKAM_HOME directory does not a valid parent directory".to_string(),
)
})?;
Ok(parent.join(format!("{dir_name}.bak")))
}
}
Expand Down Expand Up @@ -303,7 +304,7 @@ impl CliState {
Ok(get_env_with_default::<PathBuf>(
"OCKAM_HOME",
home::home_dir()
.ok_or(CliStateError::InvalidPath("$HOME".to_string()))?
.ok_or_else(|| CliStateError::InvalidPath("$HOME".to_string()))?
.join(".ockam"),
)?)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -441,9 +441,7 @@ impl ProjectRoute {
let project_id = dnsaddr
.split('.')
.next()
.ok_or(CliStateError::InvalidData(
"Invalid project route".to_string(),
))?
.ok_or_else(|| CliStateError::InvalidData("Invalid project route".to_string()))?
.to_string();
Ok(Self {
id: project_id,
Expand Down Expand Up @@ -529,11 +527,11 @@ impl EnrollmentTicket {
let project_change_history = project
.project_change_history
.as_ref()
.ok_or(ApiError::core("no project change history"))?;
.ok_or_else(|| ApiError::core("no project change history"))?;
let authority_change_history = project
.authority_identity
.as_ref()
.ok_or(ApiError::core("no authority change history"))?;
.ok_or_else(|| ApiError::core("no authority change history"))?;
let authority_route = project
.authority_access_route
.as_ref()
Expand All @@ -558,14 +556,12 @@ impl EnrollmentTicket {
let project_change_history = project
.project_change_history
.as_ref()
.ok_or(ApiError::core("no project change history in legacy ticket"))?
.ok_or_else(|| ApiError::core("no project change history in legacy ticket"))?
.clone();
let authority_change_history = project
.authority_identity
.as_ref()
.ok_or(ApiError::core(
"no authority change history in legacy ticket",
))?
.ok_or_else(|| ApiError::core("no authority change history in legacy ticket"))?
.clone();
let authority_route = project
.authority_access_route
Expand Down
32 changes: 16 additions & 16 deletions implementations/rust/ockam/ockam_api/src/cli_state/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,11 +369,7 @@ impl CliState {
.nodes_repository()
.get_default_node()
.await?
.ok_or(Error::new(
Origin::Api,
Kind::NotFound,
"There is no default node",
))?)
.ok_or_else(|| Error::new(Origin::Api, Kind::NotFound, "There is no default node"))?)
}

/// Return the node information for the given node name, otherwise for the default node
Expand Down Expand Up @@ -416,11 +412,13 @@ impl CliState {
}
})
.max_by_key(|file| file.metadata().unwrap().modified().unwrap())
.ok_or(Error::new(
Origin::Api,
Kind::NotFound,
format!("there is no log file for the node {node_name}"),
))?;
.ok_or_else(|| {
Error::new(
Origin::Api,
Kind::NotFound,
format!("there is no log file for the node {node_name}"),
)
})?;
Ok(current_log_file.path())
}
}
Expand Down Expand Up @@ -625,11 +623,13 @@ impl NodeInfo {
Ok(self
.tcp_listener_address
.as_ref()
.ok_or(ockam::Error::new(
Origin::Api,
Kind::Internal,
"no transport has been set on the node".to_string(),
))
.ok_or_else(|| {
ockam::Error::new(
Origin::Api,
Kind::Internal,
"no transport has been set on the node".to_string(),
)
})
.and_then(|t| t.multi_addr())?)
}

Expand Down Expand Up @@ -662,7 +662,7 @@ impl NodeInfo {
pub fn status(&self) -> NodeProcessStatus {
if let Some(pid) = self.pid() {
let mut sys = System::new();
sys.refresh_processes(ProcessesToUpdate::All, false);
sys.refresh_processes(ProcessesToUpdate::Some(&[Pid::from_u32(pid)]), false);
davide-baldo marked this conversation as resolved.
Show resolved Hide resolved
if let Some(p) = sys.process(Pid::from_u32(pid)) {
// Under certain circumstances, the process can be in a state where it's not running,
// and we are unable to kill it. For example, `kill -9` a process created by
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ impl CliState {
.tcp_portals_repository()
.get_tcp_inlet(node_name, alias)
.await?
.ok_or(ockam_core::Error::new(
Origin::Api,
Kind::NotFound,
format!("no tcp inlet found for node {node_name}, with alias {alias}"),
))?)
.ok_or_else(|| {
ockam_core::Error::new(
Origin::Api,
Kind::NotFound,
format!("no tcp inlet found for node {node_name}, with alias {alias}"),
)
})?)
}

/// Delete a TCP inlet
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ impl CliState {
/// Return a random root directory
pub fn test_dir() -> Result<PathBuf> {
Ok(home::home_dir()
.ok_or(CliStateError::InvalidPath("$HOME".to_string()))?
.ok_or_else(|| CliStateError::InvalidPath("$HOME".to_string()))?
.join(".ockam")
.join(".tests")
.join(random_name()))
Expand Down
26 changes: 15 additions & 11 deletions implementations/rust/ockam/ockam_api/src/cli_state/trust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,13 @@ impl CliState {
};

let authority_route =
multiaddr_to_transport_route(authority_multiaddr).ok_or(Error::new(
Origin::Api,
Kind::NotFound,
format!("Invalid authority route: {}", &authority_multiaddr),
))?;
multiaddr_to_transport_route(authority_multiaddr).ok_or_else(|| {
Error::new(
Origin::Api,
Kind::NotFound,
format!("Invalid authority route: {}", &authority_multiaddr),
)
})?;
let info = RemoteCredentialRetrieverInfo::create_for_project_member(
authority_identifier.clone(),
authority_route,
Expand Down Expand Up @@ -112,14 +114,16 @@ impl CliState {
) -> Result<NodeManagerTrustOptions> {
let authority_identifier = project
.authority_identifier()
.ok_or(ApiError::core("no authority identifier"))?;
.ok_or_else(|| ApiError::core("no authority identifier"))?;
let authority_multiaddr = project.authority_multiaddr()?;
let authority_route =
multiaddr_to_transport_route(authority_multiaddr).ok_or(Error::new(
Origin::Api,
Kind::NotFound,
format!("Invalid authority route: {}", &authority_multiaddr),
))?;
multiaddr_to_transport_route(authority_multiaddr).ok_or_else(|| {
Error::new(
Origin::Api,
Kind::NotFound,
format!("Invalid authority route: {}", &authority_multiaddr),
)
})?;

let project_id = project.project_id().to_string();
let project_member_retriever = NodeManagerCredentialRetrieverOptions::Remote {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ impl CreateServiceInvitation {
recipient_email: recipient_email.clone(),
project_identity: project
.project_identifier()
.ok_or(ApiError::core("no project identifier"))?,
.ok_or_else(|| ApiError::core("no project identifier"))?,
project_route: project.project_multiaddr()?.to_string(),
project_authority_identity: project
.authority_identifier()
.ok_or(ApiError::core("no authority identifier"))?,
.ok_or_else(|| ApiError::core("no authority identifier"))?,
project_authority_route: project_authority_route.to_string(),
shared_node_identity: node_identifier,
shared_node_route: service_route.as_ref().to_string(),
Expand Down
Loading
Loading