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

Write to an existing String buffer instead of allocating a new one #90

Open
P-E-Meunier opened this issue Jun 25, 2017 · 9 comments
Open
Assignees
Milestone

Comments

@P-E-Meunier
Copy link

So, I'm trying to use maud for nest.pijul.com. So far I like it, but nest.pijul.com runs on exactly four threads, forked in the beginning, and is entirely nonblocking. More specifically:

  • My async Postgres crate starts exactly one connection to the server per thread, i.e. currently four connections, independent from the number of clients or requests. It allocates two buffers per thread, once, and never reallocates or drops them.
  • My async HTTP server allocates two buffers per connection, and reuses them if you use HTTP keep-alive.
  • An SSH server allocating a constant number of buffers (like 8) per connection.

At least one buffer per connection is not really avoidable if you don't want to mix requests between clients, but the internals of a server should not need that.

So, in this context, Maud has the potential to allocate a single buffer per thread, and reuse it between clients, because it is never interrupted by IO. In a standard synchronous server, I agree with your comments on benchmarks (you would need some number of buffers per client), but async IO can allow you to have a constant memory use and constant number of allocations.

@lambda-fairy
Copy link
Owner

lambda-fairy commented Jul 2, 2017

Great to hear that you like it :)

Note that the Render trait has a .render_to() method, which appends to an existing buffer. The code generator uses this method by default, so there's nothing to worry about on that front as long as .render_to() itself is written efficiently.

The issue, then, would be with the html! macro itself. As you've noticed, every call to html! allocates a new buffer, which is less than optimal when you control the event loop. To solve this problem, we would need to either (a) take an output buffer as a parameter, or (b) wrap the generated code in a closure that does the same.

In my experience, either of these solutions will make Maud less ergonomic to use. I also think that for most users this trade-off is not worth it for them. (Maybe my opinion is wrong here, given that these users also chose Rust!) So if we do end up adding a no-allocation option for Maud, I think it should at least be optional.

@lambda-fairy
Copy link
Owner

lambda-fairy commented Jul 2, 2017

I wonder if we could use trait overloading to pick between the two approaches based on context. Something like this:

trait FromTemplate<F: FnOnce(&mut String)> {
    fn from_template(template: F) -> Self;
}

impl FromTemplate<F: FnOnce(&mut String)> for Markup {
    fn from_template(template: F) -> Markup {
        let mut buffer = String::new();
        template(buffer);
        PreEscaped(buffer)
    }
}

struct MarkupFn<F>(F);

impl FromTemplate<F: FnOnce(&mut String)> for MarkupFn<F> {
    // ...
}

impl Render<F: FnOnce(&mut String)> for MarkupFn<F> {
    // ...
}

I'm not sure how the Render impl for MarkupFn would work though, given that the former takes &self. I guess we can change the trait to move the value instead.


I'm also wary of breaking control flow. For example, currently we can do this:

fn query_database() -> impl Iterator<Item=Result<DbRow, DbError>> { ... }

let result = html! {
    @for entry in query_database() {
        li (entry?)
    }
};

where the ? operator will break out of the enclosing scope.

If we wrap the generated code in a closure then this pattern will no longer work.

@P-E-Meunier
Copy link
Author

I've not looked at how maud is implemented, so this might be a stupid suggestion, but how about splitting the html! into two different macros?

For instance, html_with_buffer!(buffer, …) and html!(…).

either of these solutions will make Maud less ergonomic to use

I agree for the solution with closures, but not for buffers. The best crates I've ever used let me know where allocations are made. This is more ergonomic:

  • the small short-term loss in ergonomy is not clear to me, since I usually start with a working example of calls to a crate, that I modify and expand. I believe most people do the same, which is why cargo doc encourages you to show examples.

  • in the long run, this actually makes the library more ergonomic, since you spend less time profiling and optimising when you know you can have full control. Also, more control with Rust's safety guarantees tend to make me feel more powerful (that might be just me, but I've heard similar reports by others, for instance about tokio).

@lambda-fairy
Copy link
Owner

Yep, I think you have a good point there.

I'd be okay with adding a html_to!(buffer, ...) macro, once the dust settles on #92. (I prefer this name because it's consistent with render_to.)

@lambda-fairy
Copy link
Owner

lambda-fairy commented Jul 30, 2017

Okay -- since #92 has landed now, I'll be happy to take a PR to implement an html_to! macro as described above. The relevant code is in maud_macros/src/lib.rs.

@lambda-fairy lambda-fairy changed the title Default implementation of render with asynchronous http Implement an html_to! macro that writes to an existing buffer instead of allocating a new one Jul 30, 2017
@lambda-fairy lambda-fairy changed the title Implement an html_to! macro that writes to an existing buffer instead of allocating a new one Implement an html_to! macro that writes to an existing String buffer instead of allocating a new one Jul 30, 2017
@lambda-fairy
Copy link
Owner

@P-E-Meunier I've changed the title of the issue -- does this sound like what you're looking for?

I haven't done much async I/O work in Rust so I want to confirm that this addresses your use case.

@P-E-Meunier
Copy link
Author

Yes, it does address my use case (the goal is to allocate a single buffer for all pages served during the entire life of the process).

@sanmai-NL
Copy link
Contributor

sanmai-NL commented Oct 15, 2018

This could help lessen the need for #90, since it removes the need to mention a String type in https://github.com/lfairy/maud/blob/365d0ab956c8cf5db9d027c11b052d7e201e63d6/maud_macros/src/lib.rs#L50-L55.

@lambda-fairy lambda-fairy self-assigned this Oct 3, 2020
@lambda-fairy lambda-fairy removed the easy label Oct 3, 2020
@lambda-fairy lambda-fairy changed the title Implement an html_to! macro that writes to an existing String buffer instead of allocating a new one Write to an existing String buffer instead of allocating a new one Oct 7, 2020
@lambda-fairy lambda-fairy added this to the 1.0 milestone Apr 24, 2021
@badicsalex
Copy link

badicsalex commented Nov 5, 2022

Hi,

I found myself writing the following code:

impl<'a> maud::Render for MyLittleStructure {
   fn render(&self) -> maud::Markup {
        html!(
            .container {
                ... Non trivial html stuff ...
            }
        )
    }
}

Since I'm rendering something like 16000 divs, I worry that this is going to be a performance bottleneck sooner or later.

I'd love to write something like the following:

impl<'a> maud::Render for MyLittleStructure {
    fn render_to(&self, buffer: &mut String) {
        html_to!(buffer,
            .container {
                ... Non trivial html stuff ...
            }
        )
    }
}

Are you still accepting PRs for this feature? I think this could be done very easily with the current state of the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants