Skip to content

feat: use rc to avoid allocating a new string for extension_id #615

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

feat: use rc to avoid allocating a new string for extension_id #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 5 additions & 6 deletions lambda-extension/src/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::{rc::Rc};

use serde::Deserialize;

/// Request tracing information
Expand Down Expand Up @@ -56,16 +58,13 @@ impl NextEvent {
/// event that the Lambda Runtime is going to process
pub struct LambdaEvent {
/// ID assigned to this extension by the Lambda Runtime
pub extension_id: String,
pub extension_id: Rc<String>,
/// Next incoming event
pub next: NextEvent,
}

impl LambdaEvent {
pub(crate) fn new(ex_id: &str, next: NextEvent) -> LambdaEvent {
LambdaEvent {
extension_id: ex_id.into(),
next,
}
pub(crate) fn new(extension_id: Rc<String>, next: NextEvent) -> Self {
LambdaEvent { extension_id, next }
}
}
6 changes: 4 additions & 2 deletions lambda-extension/src/extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{
convert::Infallible, fmt, future::ready, future::Future, net::SocketAddr, path::PathBuf, pin::Pin, sync::Arc,
convert::Infallible, fmt, future::ready, future::Future, net::SocketAddr, path::PathBuf, pin::Pin, rc::Rc,
sync::Arc,
};

use hyper::{server::conn::AddrStream, Server};
Expand Down Expand Up @@ -221,6 +222,7 @@ where

let extension_id = register(client, self.extension_name, self.events).await?;
let extension_id = extension_id.to_str()?;
let extension_id_rc = Rc::new(String::from(extension_id));
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't String::from(&str) an allocation anyways? can we make the extension_id field an &str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • yes you're right but it's done only once (outside the loop) instead of doing it for every received events
  • I tried to make it a &str but this introduces a lot of lifetime-related code, was a bit less readable (IMHO) and also introduces a breaking change that's why I went with the Rc<String> path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think @greenwoodcm?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i think that seems reasonable. is this change non-breaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no it's not, I put a warning in the description but this basically modifies the public extension_id field in LambdaEvent from String to Rc<String>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does it make sense to you @greenwoodcm ?

let mut ep = self.events_processor;

if let Some(mut log_processor) = self.logs_processor {
Expand Down Expand Up @@ -315,7 +317,7 @@ where
let event: NextEvent = serde_json::from_slice(&body)?;
let is_invoke = event.is_invoke();

let event = LambdaEvent::new(extension_id, event);
let event = LambdaEvent::new(extension_id_rc.clone(), event);

let ep = match ep.ready().await {
Ok(ep) => ep,
Expand Down