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

Use maud templates and serde_json for directory listings #367

Merged
merged 7 commits into from
Apr 27, 2024

Conversation

palant
Copy link
Contributor

@palant palant commented Apr 25, 2024

Description

This is essentially the same as #365 but using maud templates. Maud provides a cleaner syntax than sailfish (IMHO), and it doesn’t masquerade as being separate from the code (sailfish templates also use variables which are in scope even when these are outside the template). Local testing indicates no performance difference here either. But unlike sailfish, using maud doesn’t result in additional whitespace.

Downside: maud is for HTML only, JSON logic remains as it was. Then again, maybe it’s better to just go to serde_json for a cleaner JSON generation approach.

How Has This Been Tested?

Integration tests pass. Functional testing on Linux is also fine.

Copy link

semanticdiff-com bot commented Apr 25, 2024

Review changes with SemanticDiff.

Analyzed 1 of 3 files.

Overall, the semantic diff is 5% smaller than the GitHub diff.

Filename Status
Cargo.lock Unsupported file format
Cargo.toml Unsupported file format
✔️ src/directory_listing.rs 4.59% smaller

@joseluisq joseluisq added v2 v2 release enhancement New feature or request labels Apr 25, 2024
@joseluisq
Copy link
Collaborator

JSON logic remains as it was. Then again, maybe it’s better to just go to serde_json for a cleaner JSON generation approach.

Yes, serde_json can do the job, go for it.

@palant
Copy link
Contributor Author

palant commented Apr 26, 2024

Yes, serde_json can do the job, go for it.

Separate PR once this is merged then.

@palant
Copy link
Contributor Author

palant commented Apr 27, 2024

Added change to the JSON-formatted directory listings to this PR after all, would have been a fat merge conflict otherwise. Performance is minimally improved in local tests, around 2%.

Originally I used an intermediate data structure for JSON serialization. It made sense to serialize FileEntry directly however. So some fields had to be renamed and types changed here.

@palant
Copy link
Contributor Author

palant commented Apr 27, 2024

Added change to the JSON-formatted directory listings to this PR after all, would have been a fat merge conflict otherwise. Performance is minimally improved in local tests, around 2%.

This caused an unexpected performance regression for HTML listings, both large and small. This makes little sense, trying to figure out what’s going on there.

@palant
Copy link
Contributor Author

palant commented Apr 27, 2024

It’s a performance regression that cannot be reproduced locally. And flamegraph only shows that a bulk of the time is taken up by chrono and humansize calls but these didn’t change.

@palant
Copy link
Contributor Author

palant commented Apr 27, 2024

Of course I cannot reproduce a performance regression locally if it isn’t caused by this change. 🙄

The performance regression first shows up when current development is merged into this branch. Looking into the changes, #360 appears to be the culprit.

@joseluisq
Copy link
Collaborator

The performance regression first shows up when current development is merged into this branch. Looking into the changes, #360 appears to be the culprit.

Yeah, It can be due to the dynamic compression.

What I noted in the benchmarks is that the regression is notorious when listing large index. E.g. ~48% latency increase compared to for nginx. But in other tests, we are almost closer to the first two servers.

@palant
Copy link
Contributor Author

palant commented Apr 27, 2024

What I noted in the benchmarks is that the regression is notorious when listing large index. E.g. ~48% latency increase compared to for nginx. But in other tests, we are almost closer to the first two servers.

That’s not because of the compression, has nothing to do with this “regression.” We are simply pretty slow at formatting file sizes and modification times.

@joseluisq joseluisq changed the title Use maud templates for directory listings Use maud templates and serde_json for directory listings Apr 27, 2024
Copy link
Collaborator

@joseluisq joseluisq left a comment

Choose a reason for hiding this comment

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

Thanks!

@joseluisq joseluisq added the dependency Related to dependencies label Apr 27, 2024
@joseluisq joseluisq merged commit 5b5ea98 into static-web-server:master Apr 27, 2024
36 checks passed
@palant palant deleted the maud branch April 27, 2024 21:59
@joseluisq joseluisq added this to the v2.30.0 milestone Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency Related to dependencies enhancement New feature or request v2 v2 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants