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

Cache parsing templates #59

Merged
merged 7 commits into from
Jul 11, 2024
Merged

Conversation

Kijewski
Copy link
Collaborator

@Kijewski Kijewski commented Jul 9, 2024

It compiles, tests work just fine, but I did not benchmark yet if the caching is of any use performance-wise.

@Kijewski
Copy link
Collaborator Author

Kijewski commented Jul 9, 2024

It might well be that we have to convert our Rcs to Arcs to get any performance gains, if rust does not call the derive macro in the same thread. Arcs are ridiculously slow compared to Rcs, but we could use triomphe to mitigate the slow downs a bit.

@GuillaumeGomez
Copy link
Contributor

Better bench in any case to ensure any change you make going forward is positive (but you already know that, just love to state the obvious 😆 ).

@Kijewski
Copy link
Collaborator Author

Kijewski commented Jul 9, 2024

Well, there's a tiny performance regression:

$ cd rinja_derive_standalone; cargo bench
hello_world             time:   [706.57 µs 774.11 µs 836.25 µs]
                        change: [+891.79% +965.79% +1053.5%] (p = 0.00 < 0.05)
                        Performance has regressed.

item_info.html          time:   [805.59 µs 852.78 µs 903.00 µs]
                        change: [+982.26% +1020.3% +1066.4%] (p = 0.00 < 0.05)
                        Performance has regressed.

ctrl+C

Not using any includes, etc., the cache is useless in this benchmark, of course, but I have no idea why the slow down are this terrible. :/

}
}
Ok(entry.insert(source.into()).clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be better to replace clone calls (here and above) with Rc::clone. Not useful for performance, but better for reading. :)

}
}

thread_local! {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thought that it's not really useful to keep these statics as thread local.

@GuillaumeGomez
Copy link
Contributor

Not using any includes, etc., the cache is useless in this benchmark, of course, but I have no idea why the slow down are this terrible. :/

Well, it's still less than 1ms. So not really that important I think. Still be interesting to try to understand why these two statics are so bad at this overall.

Also would be interesting to have test cases using a lot of include.

@Kijewski
Copy link
Collaborator Author

Kijewski commented Jul 9, 2024

Also would be interesting to have test cases using a lot of include.

Yeah, but I have no idea how to easily implement this. :/ I guess we could use vfs to have a fake file-system, but implementing that would be kinda difficult I guess, and I'd defer that task to somewhen in the future.

@Kijewski Kijewski force-pushed the pr-cache-ast branch 3 times, most recently from 31c6c56 to 6918c88 Compare July 10, 2024 06:58
@Kijewski
Copy link
Collaborator Author

Rinja_derive_standalone's benchmarks are useless now: We only measure how much faster the cache is than parsing. The parsing itself is not benchmarked anymore because it's part of the warm-up phase.

Benchmark results
hello_world             time:   [46.368 µs 46.431 µs 46.492 µs]
                        change: [-9.6500% -9.0053% -8.3628%] (p = 0.00 < 0.05)
                        Performance has improved.

item_info.html          time:   [56.031 µs 56.168 µs 56.293 µs]
                        change: [-18.971% -18.687% -18.425%] (p = 0.00 < 0.05)
                        Performance has improved.

item_union.html         time:   [141.11 µs 141.43 µs 141.80 µs]
                        change: [-26.505% -26.226% -25.940%] (p = 0.00 < 0.05)
                        Performance has improved.

page.html               time:   [607.29 µs 608.38 µs 609.53 µs]
                        change: [-24.460% -24.242% -23.995%] (p = 0.00 < 0.05)
                        Performance has improved.

print_item.html         time:   [130.40 µs 130.81 µs 131.36 µs]
                        change: [-20.143% -19.854% -19.521%] (p = 0.00 < 0.05)
                        Performance has improved.

short_item_info.html    time:   [111.89 µs 112.10 µs 112.34 µs]
                        change: [-21.130% -20.563% -19.876%] (p = 0.00 < 0.05)
                        Performance has improved.

sidebar.html            time:   [169.74 µs 170.02 µs 170.32 µs]
                        change: [-24.447% -24.216% -23.991%] (p = 0.00 < 0.05)
                        Performance has improved.

What you would expect: "hello world" with only one expression is only mildly faster, and the more complex a template, the bigger the speed-up.

GuillaumeGomez
GuillaumeGomez previously approved these changes Jul 10, 2024
@GuillaumeGomez
Copy link
Contributor

Changes are much smaller, nicely done. :)

However, the MSRV seems to be 1.71. So just need to update it and then it's all good!

@Kijewski
Copy link
Collaborator Author

Caching reading the configuration is still missing, then it's ready. :)

@Kijewski Kijewski force-pushed the pr-cache-ast branch 2 times, most recently from e1b55dc to 344b492 Compare July 10, 2024 19:36
@GuillaumeGomez
Copy link
Contributor

I have to admit I didn't even think about the config file. Good catch! Please ping me once it's ready for ready.

@Kijewski

This comment was marked as outdated.

@Kijewski Kijewski force-pushed the pr-cache-ast branch 3 times, most recently from e2079ba to 47acc3c Compare July 10, 2024 23:15
`quick_cache` is the wrong tool for this job. It want to evict items
after a time, which must not happen. It has too many options and
features we don't need.
@Kijewski Kijewski marked this pull request as ready for review July 11, 2024 03:41
@Kijewski
Copy link
Collaborator Author

It's ready to review! :) The last commit is a overhaul of the previous three commits, but it would be terror to refactor them. I hope that's okay! I think I made the diff as small as possible.

@GuillaumeGomez
Copy link
Contributor

Nah it's fine, I don't care. 😆

Thanks a lot!

@GuillaumeGomez GuillaumeGomez merged commit 0e971b5 into rinja-rs:master Jul 11, 2024
17 checks passed
@Kijewski Kijewski deleted the pr-cache-ast branch July 12, 2024 04:41
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