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

Potential escaping optimization #307

Open
agentgt opened this issue Dec 19, 2023 · 7 comments
Open

Potential escaping optimization #307

agentgt opened this issue Dec 19, 2023 · 7 comments

Comments

@agentgt
Copy link

agentgt commented Dec 19, 2023

@casid as promised I wanted to share with you some potential optimizations I found while modernizing JMustache (a runtime based Mustache engine) escaping.

https://github.com/jstachio/escape-benchmark

The benchmark just tests escaping algorithms and not full template engines.

The benchmark and algorithm of StreamEscaperBenchmark.streamSubstring2 could possibly be put in JTE. I think currently JTE is analogous to StreamEscaperBenchmark.streamCharAt but without the inner loop. JStachio I believe is closest to StreamEscaperBenchmark.streamSubstring at the moment.

Anyway feel free to close the issue or move to discussion but wanted to share the benchmark repo in case you might find it interesting.

@casid
Copy link
Owner

casid commented Dec 20, 2023

Thanks for sharing.

I'm quite happy with the current jte implementation though. It is very easy to understand, quite fast and does not do any memory allocations. https://github.com/casid/jte/blob/main/jte-runtime/src/main/java/gg/jte/html/escape/Escape.java

@casid casid closed this as completed Dec 20, 2023
@agentgt
Copy link
Author

agentgt commented Dec 20, 2023

FWIW it’s unlikely I will change jstachio escaper (which appears to be similar to JTE) based on the benchmark.

I just thought it was bizarre an array lookup beat a switch.

@agentgt
Copy link
Author

agentgt commented Dec 20, 2023

@casid

I improved the documentation of the benchmark so if you can check it one last time.

JStachio and JTE do: StreamEscaperBenchmark.streamSubstringInlineSwitch. jstachio version (EDIT to link to original inline switch version) which I assume you are familiar with as you found the missing escape characters security bug (thank you again btw!). I'm not sure if you were influenced by JStachio escaping given the timing of found bug and when you rewrote escaping so I feel obligated to say I never benchmarked it but now have.

It was one of the worst performers and is why I kind of reached out to you.

I am not sure why switch expressions are so slow.

@casid
Copy link
Owner

casid commented Dec 21, 2023

Thanks for the update @agentgt.

I followed IntelliJ's recommendation and migrated to the new switch expression, when jte 3 required Java 17 as minimum version. If I read your benchmark results correctly, changing this to an old fashioned switch doubles the throughput? That's a change worth considering!

@casid casid reopened this Dec 21, 2023
@agentgt
Copy link
Author

agentgt commented Dec 21, 2023

I'm still testing across various machines and operating systems. It appears true Intel x64 machines are smart enough to make the inline switch expression fast and in some cases faster than the array lookup.

But AMD, and Apple Silicon (arm64) do not seem to optimize for it.

The reason I did the benchmark was because JMustache current escaping is very slow but I knew if I changed it I needed proof for the PR that whatever I switched to would be better (while JMustache does not have a lot of github stars it has an enormous amount of usage).

If inline switch expression was on par than that would have been more reason to base on JDK 17 instead of JDK 9 that JMustache is currently on.

Anyway I switched to the array lookup for JStachio to be consistent with JMustache as JMustache is kind of JStachio's reflective brother.

@agentgt
Copy link
Author

agentgt commented Dec 21, 2023

Here are some of the results:

https://github.com/jstachio/escape-benchmark/blob/main/results.md

If you have spare cycles (pun intended) maybe test on your Windows Beast 😄

I have a surprisingly lack of access to Genuine Intel machines.

@casid
Copy link
Owner

casid commented Jan 1, 2024

Thanks for the update @agentgt.

I followed IntelliJ's recommendation and migrated to the new switch expression, when jte 3 required Java 17 as minimum version. If I read your benchmark results correctly, changing this to an old fashioned switch doubles the throughput? That's a change worth considering!

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

No branches or pull requests

2 participants