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

Generator: reduce amount of intermediate allocations #11

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

Kijewski
Copy link
Collaborator

No need to allocate a String, just to write it into another String afterwards. This PR replaces the usages for format!() with format_args!().

No need to allocate a String, just to write it into another String
afterwards. This PR replaces the usages for `format!()` with
`format_args!()`.
buf.writeln("const MIME_TYPE: &'static ::std::primitive::str = ");
buf.writeln(&format!("{:?}", &self.input.mime_type));
buf.writeln(";");
buf.writeln(format_args!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't it be replaced with the writeln macro?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

format_args_nl!() is not stable, yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I think I misunderstood you. Answering in the next comment.

buf_expr.write("&");
buf_expr.write(entry.key());
buf_expr.writeln(",");
buf_expr.write(format_args!("expr{id} = &{},", entry.key()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't it be replaced with the write! macro?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure, but I think that would have a negative performance impact. We only work on Strings, but write!() works on &dyn Write. I don't know if the compiler is still able to optimize the code, not to employ vtables, is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, godbolt could answer that, let me check.

Copy link
Contributor

@GuillaumeGomez GuillaumeGomez Jun 18, 2024

Choose a reason for hiding this comment

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

I tried with this code:

use std::fmt::Write;

// If you use `main()`, declare it as `pub` to see it in the output:
pub fn main() -> usize {
    let mut s = String::new();
    let a = "a";
    let b = 12;

    s.write_fmt(format_args!("{a} {b}"));
    write!(s, "{a} {b}");
    0
}

I compiled with -C opt-level=3. So for s.write_fmt(format_args!("{a} {b}")); we have:

        mov     qword ptr [rsp + 40], rbx
        lea     r14, [rip + <&T as core::fmt::Display>::fmt::h0e293c54e2c02aa4]
        mov     qword ptr [rsp + 48], r14
        lea     r15, [rsp + 12]
        mov     qword ptr [rsp + 56], r15
        mov     r12, qword ptr [rip + core::fmt::num::imp::<impl core::fmt::Display for i32>::fmt::h4d2c4c91afac718c@GOTPCREL]
        mov     qword ptr [rsp + 64], r12

and for write!(s, "{a} {b}"); we have:

        call    qword ptr [rip + core::fmt::write::hc090a2ffd6b28c4a@GOTPCREL]
        mov     qword ptr [rsp + 40], rbx
        mov     qword ptr [rsp + 48], r14
        mov     qword ptr [rsp + 56], r15
        mov     qword ptr [rsp + 64], r12

So at this point, I think only a benchmark would be able to differentiate both.

EDIT: although it seems like the write/writeln macros seems to generate less asm code (which is generally better), but as always with computers, in the end only benches comparison can give the final word.

@GuillaumeGomez
Copy link
Contributor

That makes me think that we should add a way to bench performance impact somehow (not memory, too unlikely to be useful).

@Kijewski
Copy link
Collaborator Author

Is it possible to (repeatedly) call rinja_generator::derive_template() in "normal" code? If so, then I guess benchmarking would be easy. Otherwise, I guess it would be mostly a benchmark for your harddisk and the filesystem cache. :/

@GuillaumeGomez
Copy link
Contributor

I thought we could use something like we have here. We can do an include_str! and then call the parser on it in the bench I think?

Copy link
Contributor

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

I think it'd be interesting to give a try to write! and writeln! macros but the current code is already better than the existing so can be done in a follow-up!

@GuillaumeGomez GuillaumeGomez merged commit 0d0ede1 into rinja-rs:master Jun 18, 2024
16 checks passed
@GuillaumeGomez
Copy link
Contributor

I opened #12 and #13 so it's not forgotten. I think it's really minor improvements so not a priority at all, but could still be interesting. :)

@Kijewski Kijewski deleted the pr-less-alloc branch June 18, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants