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

Make |tojson and few more filters faster #1008

Merged
merged 12 commits into from
May 1, 2024
Merged

Make |tojson and few more filters faster #1008

merged 12 commits into from
May 1, 2024

Conversation

Kijewski
Copy link
Contributor

@Kijewski Kijewski commented Apr 17, 2024

This proof-of-concept PR changes the filter |tojson so that it does not collect the serialized data into a string, but writes the data into the target writer directly.

This makes the benchmark run 81% (only serializing) or 39% (serializing and escaping for HTML) faster. The benchmarked data is not a fair representation for the data you would most likely escape, though.

@Kijewski Kijewski marked this pull request as draft April 17, 2024 21:15
@Kijewski
Copy link
Contributor Author

Also, I think the default |tojson filter should not prettify the JSON data. I guess it's unlikely that you want to show a JSON representation to the user, and it's much more likely that you want to fill some Javascript variable, never to be seen by the user.

@Kijewski Kijewski changed the title PoC: make filters faster Make |tojson filter faster Apr 17, 2024
@Kijewski Kijewski marked this pull request as ready for review April 17, 2024 22:12
askama/src/error.rs Outdated Show resolved Hide resolved
askama/src/filters/json.rs Show resolved Hide resolved
askama/src/filters/json.rs Show resolved Hide resolved
@Kijewski Kijewski changed the title Make |tojson filter faster Make |tojson and few more filters faster Apr 17, 2024
@djc
Copy link
Owner

djc commented Apr 18, 2024

Also, I think the default |tojson filter should not prettify the JSON data. I guess it's unlikely that you want to show a JSON representation to the user, and it's much more likely that you want to fill some Javascript variable, never to be seen by the user.

What are the costs of making JSON pretty? I'm inclined to think the cost of JSON encoding will probably not show up for people and in most contexts pretty JSON will be helpful more than it will be harmful (because of increased size). So there's a trade-off:

  1. Performance cost of rendering pretty vs compact
  2. Bandwidth cost of transmitting pretty vs compact
  3. Developer experience benefits of rendering pretty vs compact

I would argue that (3) is likely to outstrip (1) and (2), but obviously it depends on the situation.

(To be clear, I'm arguing that you might want to look at your HTML/JS source code sometimes and when you do, it will be nice if you can follow the general structure of the JSON code, even if the JSON is not shown to the actual user.)

@Kijewski
Copy link
Contributor Author

Let's keep |tojson pretty for now, and maybe discuss in a new issue if we should change it later?

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Great stuff! Have a bunch of suggestions but nothing major.

askama/src/error.rs Show resolved Hide resolved
askama_escape/src/lib.rs Show resolved Hide resolved
askama/src/filters/mod.rs Outdated Show resolved Hide resolved
askama/src/filters/mod.rs Outdated Show resolved Hide resolved
askama/src/filters/json.rs Outdated Show resolved Hide resolved
askama/src/filters/mod.rs Outdated Show resolved Hide resolved
askama/src/filters/mod.rs Outdated Show resolved Hide resolved
This PR changes the filter `|tojson` so that it does not collect the
serialized data into a string, but writes the data into the target
writer directly.

This makes the benchmark run 81% (only serializing) or 39% (serializing
and escaping for HTML) faster. The benchmarked data is not a fair
representation for the data you would most likely serialize, though.
@Kijewski
Copy link
Contributor Author

So, I let all filters return impl fmt::Display. This has the added advantage that we can change the implementation of e.g. |trim without it being a semver incompatible change.

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks, great work!

@djc djc merged commit 8d3a3f7 into djc:main May 1, 2024
16 checks passed
@Kijewski Kijewski deleted the pr-faster-filters branch May 1, 2024 09:13
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.

3 participants