feat: use rc to avoid allocating a new string for extension_id #615
Conversation
| @@ -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)); | |||
There was a problem hiding this comment.
isn't String::from(&str) an allocation anyways? can we make the extension_id field an &str?
There was a problem hiding this comment.
- 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
&strbut 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 theRc<String>path
There was a problem hiding this comment.
what do you think @greenwoodcm?
There was a problem hiding this comment.
ok i think that seems reasonable. is this change non-breaking?
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
does it make sense to you @greenwoodcm ?
There was a problem hiding this comment.
The extension id is a random string assigned to an extension when it's registered by the runtime to accompany a function. Every time the extension is registered, it receives a new random id.
When I added that field to the event, I thought it could be useful at some point. After further thinking, I don't believe it's really useful for anything worth exposing externally, it should just be an internal implementation detail.
Since this is going to be a breaking change anyways, I'd be more in favor of removing it completely.
Issue #, if available: potential fix for #614
Description of changes:
Using a RC around the extension id.
Now it clones the RC for each event which is way lighter than cloning the full extension_id string
By submitting this pull request