djc / askama

Type-safe, compiled Jinja-like templates for Rust
Apache License 2.0
3.25k stars 213 forks source link

Let AST nodes own their String content #1041

Closed Kijewski closed 2 months ago

Kijewski commented 2 months ago

This enables having nodes in the same AST that were produced from multiple source files.

Essentially: %s/&str/RcStr/g

This is a draft. For now, I removed all the parser tests, but will re-add them later.

👍🏻 What I like about this PR: The lifetimes are gone, which makes the code much easier on the eyes.

👎🏻 What I dislike about this PR: The lifetimes are gone, so I have to touch every other line of the source code to remove any 'a.

Do you think the PR is worth investing more time into? I hate that I had to change so many lines; this makes the change extremely un-reviewable and makes it difficult to git blame in the future.

The change is part of #1034. I want to do the inclusions directly in the parsing step, not only when generating code. Just as if you replaced {% include "foo.html" %} with the content of foo.html in the template file.

GuillaumeGomez commented 2 months ago

If this change, if a template file is updated, will it trigger a new compilation (as it should)?

djc commented 2 months ago

Have you benchmarked the performance impact on parsing?

If the goal is to have nodes from different sources in the same AST, it's not obvious to me that this is the best way to get there. At least in theory we can just keep Strings for each source on the stack and keep an AST containing references to any of these sources with the same lifetime, right?

Kijewski commented 2 months ago

Have you benchmarked the performance impact on parsing?

I didn't, but I daresay the change is not for free. Rc::clone has a check to guard against integer overflows like loop { forget(rc.clone()); }which adds stack unwinding code.

At least in theory we can just keep Strings for each source on the stack and keep an AST containing references to any of these sources with the same lifetime, right?

Sounds like a good idea! Simply have a Vec<String> of sources. When merging two ASTs, we simply move the sources.

Kijewski commented 2 months ago
Benchmarked using #1043 ```text librustdoc/all time: [1.1175 ms 1.1192 ms 1.1212 ms] thrpt: [12.594 MiB/s 12.618 MiB/s 12.636 MiB/s] change: time: [+175.67% +177.58% +179.81%] (p = 0.00 < 0.05) thrpt: [-64.262% -63.974% -63.725%] Performance has regressed. librustdoc/item_info time: [20.457 µs 20.704 µs 20.959 µs] thrpt: [7.5078 MiB/s 7.6004 MiB/s 7.6921 MiB/s] change: time: [+218.83% +222.20% +225.66%] (p = 0.00 < 0.05) thrpt: [-69.293% -68.963% -68.635%] Performance has regressed. librustdoc/item_union time: [95.977 µs 96.918 µs 97.914 µs] thrpt: [10.081 MiB/s 10.184 MiB/s 10.284 MiB/s] change: time: [+193.26% +197.01% +200.83%] (p = 0.00 < 0.05) thrpt: [-66.758% -66.331% -65.901%] Performance has regressed. librustdoc/page time: [498.76 µs 502.96 µs 507.32 µs] thrpt: [12.206 MiB/s 12.311 MiB/s 12.415 MiB/s] change: time: [+187.30% +190.59% +194.15%] (p = 0.00 < 0.05) thrpt: [-66.004% -65.588% -65.194%] Performance has regressed. librustdoc/print_item time: [72.098 µs 72.548 µs 73.047 µs] thrpt: [12.925 MiB/s 13.014 MiB/s 13.095 MiB/s] change: time: [+191.57% +195.25% +198.93%] (p = 0.00 < 0.05) thrpt: [-66.547% -66.130% -65.703%] Performance has regressed. librustdoc/short_item_info time: [68.824 µs 69.014 µs 69.236 µs] thrpt: [13.086 MiB/s 13.128 MiB/s 13.164 MiB/s] change: time: [+138.68% +148.91% +159.25%] (p = 0.00 < 0.05) thrpt: [-61.427% -59.824% -58.103%] Performance has regressed. librustdoc/sidebar time: [126.72 µs 126.90 µs 127.14 µs] thrpt: [9.7062 MiB/s 9.7250 MiB/s 9.7386 MiB/s] change: time: [+193.94% +198.50% +201.95%] (p = 0.00 < 0.05) thrpt: [-66.882% -66.499% -65.980%] Performance has regressed. librustdoc/source time: [61.878 µs 62.525 µs 63.239 µs] thrpt: [11.657 MiB/s 11.790 MiB/s 11.914 MiB/s] change: time: [+180.80% +185.38% +189.28%] (p = 0.00 < 0.05) thrpt: [-65.431% -64.958% -64.387%] Performance has regressed. librustdoc/type_layout_size time: [36.366 µs 36.568 µs 36.763 µs] thrpt: [7.3673 MiB/s 7.4065 MiB/s 7.4478 MiB/s] change: time: [+235.28% +237.93% +240.72%] (p = 0.00 < 0.05) thrpt: [-70.651% -70.408% -70.175%] Performance has regressed. librustdoc/type_layout time: [168.71 µs 169.83 µs 171.06 µs] thrpt: [15.738 MiB/s 15.852 MiB/s 15.958 MiB/s] change: time: [+197.93% +200.75% +203.38%] (p = 0.00 < 0.05) thrpt: [-67.038% -66.749% -66.435%] Performance has regressed. ```