-
Notifications
You must be signed in to change notification settings - Fork 84
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
Avoid allocating new Vec for every get() call #26
Comments
Also at the time most web frameworks supported the |
|
Asserted. Then this will be a refactor and change in the api. Will need to push a new major for this and correct the examples. Will do this and add the path method/docs for this in the next major release 4. |
Now .resource("/{tail:.*}", move |r| r.f({
move |r| {
let mut path: String = r.match_info().query("tail").unwrap();
if path == "" {
path = "index.html".into();
}
match Assets::get(&path) {
Some(content) => HttpResponse::Ok()
.content_type(mime_guess::guess_mime_type(path).as_ref())
.body(content.as_ref().to_vec()),
None => HttpResponse::NotFound().body("404 Not Found"),
}
}
})) But if |
Nice.. How do we solve this? |
If we change the function to return |
@vemoo any thoughts on this.
@Boscop Also why do you need to return a |
Ah, I hadn't seen the |
When in debug a |
@vemoo No, because |
@Boscop but this macro doesn't create a |
Ah right (that was already fixed), but it will still prevent a |
I think I found a better solution. We return See the updated actix example here: vemoo@74c8579 The |
I was aware of the Cow solution but I decided not to mention it because it does dynamic dispatch on the enum, whereas we don't have to do any dynamic dispatch, we can do the dispatch statically, because the case is known at compile-time. Returning impl Trait doesn't do any dynamic dispatch. |
I don't think that's dynamic dispatch. All the posible types are know at compile time. We just go from match Assets::get(&path) {
Some(Vec<u8>) => ...,
None => ...,
} in debug or match Assets::get(&path) {
Some(&'static [u8]) => ...,
None => ...,
} in release to match Assets::get(&path) {
Some(Cow::Borrowed(&'static [u8])) => ...,
Some(Cow::Owned(Vec<u8>)) => ...,
None => ...,
} both in debug and release, with the added benefit that we have access to the static lifetime, which allows using https://docs.rs/actix-web/0.7.14/actix_web/enum.Binary.html#impl-From%3C%26%27static%20%5Bu8%5D%3E which wasn't possible before. |
@vemoo Yea, by dynamic dispatch I meant the matching on the Cow.. But with the Cow solution, we'd then require client code to have essentially the same code in both cases of the Cow match (it will just choose a different instance of |
I've added |
This allocates a new Vec for every get() call:
https://github.com/pyros2097/rust-embed/blob/b509d63985c4079c0cd2aaf2e3c951ee249003d5/src/lib.rs#L62
It would be better to return a
&'static [u8]
by writing this instead:And then
pub fn get(file_path: &str) -> Option<&'static [u8]> {
The text was updated successfully, but these errors were encountered: