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

Speed up std::fmt::Write::write_str for maud::Escaper. #61

Conversation

utkarshkukreti
Copy link
Contributor

I'm seeing roughly 25% speedup on both the normal and complicated
benchmarks present in benchmarks/benches/maud.rs and
benchmarks/benches/complicated_maud.rs respectively.

Before:

test render_template ... bench:             461 ns/iter (+/- 140)
test render_complicated_template ... bench: 1,562 ns/iter (+/- 366)

After:

test render_template ... bench:             350 ns/iter (+/- 69)
test render_complicated_template ... bench: 1,145 ns/iter (+/- 633)

(The error margins are pretty high in the above snippets but I've
ran these benchmarks several times and the difference is consistent.)


What do you think @lfairy?

I also tried using str::slice_unchecked (there is no easy way to do unchecked slicing for slices yet; see rust-lang/rust#35729) but there was no measurable improvement.

I'm seeing roughly 25% speedup on both the normal and complicated
benchmarks present in `benchmarks/benches/maud.rs` and
`benchmarks/benches/complicated_maud.rs` respectively.

Before:

    test render_template ... bench:             461 ns/iter (+/- 140)
    test render_complicated_template ... bench: 1,562 ns/iter (+/- 366)

After:

    test render_template ... bench:             350 ns/iter (+/- 69)
    test render_complicated_template ... bench: 1,145 ns/iter (+/- 633)

(The error margins are pretty high in the above snippets but I've
ran these benchmarks several times and the difference is consistent.)
@lambda-fairy
Copy link
Owner

lambda-fairy commented Nov 13, 2016

How does this compare when most of the characters need to be escaped? I know that some clever approaches fall over when strings like <<<<>>>><<<<>>>>... are involved.

@TheNeikos
Copy link
Contributor

TheNeikos commented Nov 13, 2016

@lfairy Maybe that could be added to the benchmarks? An extreme case or so

@utkarshkukreti
Copy link
Contributor Author

@lfairy Good point. It's over 3.5x slower for escaping a long sequence of <> (13µs vs 3.5µs) and over 2x slower for escaping a long sequence of <foo></foo> (6.7µs vs 3µs). I guess the 2 benchmarks I compared in the commit message are the worst case for the current algorithm and best case for the new one. The question then is, what ratio of characters can we expect to require escaping in the real world and will this optimization be worth it for them?

@lambda-fairy
Copy link
Owner

lambda-fairy commented Nov 15, 2016

@TheNeikos I'd love to see a escaping benchmark! Feel free to file a PR for that.

@utkarshkukreti The values being escaped are often provided by untrusted users (e.g. comments on a blog), so we need to assume the worst case. Because of this I'm wary of approaches which vary so much based on the input. There are ways to narrow the gap (using instructions like PCMPESTRI) but they introduce a lot of complexity.

For this reason, I'm going to close this PR. Thanks for your contribution regardless!

@utkarshkukreti
Copy link
Contributor Author

@lfairy Sounds reasonable. As for PCMPESTRI, I don't think it would be faster than the current implementation if the string contains only <>&" (but it would definitely be faster than the best case of this PR).

@utkarshkukreti
Copy link
Contributor Author

With a tiny change I'm able to get a huge improvement for <><><>...: 14µs (this PR) -> 8.5µs (with diff below).

--- a/maud/src/lib.rs
+++ b/maud/src/lib.rs
@@ -181,8 +181,10 @@ impl<'a> fmt::Write for Escaper<'a> {
         for (i, &b) in bytes.iter().enumerate() {
             macro_rules! push {
                 ($str:expr) => {{
-                    unsafe {
-                        self.0.as_mut_vec().extend_from_slice(&bytes[last..i]);
+                    if i > last {
+                        unsafe {
+                            self.0.as_mut_vec().extend_from_slice(&bytes[last..i]);
+                        }
                     }
                     self.0.push_str($str);
                     last = i + 1;

The benchmark:

--- a/benchmarks/benches/maud.rs
+++ b/benchmarks/benches/maud.rs
@@ -39,3 +39,17 @@ fn render_template(b: &mut test::Bencher) {
         }
     });
 }
+
+#[bench]
+fn render_escape(b: &mut test::Bencher) {
+    // let text = "<foo></foo>";
+    let text = "<><><><><>";
+
+    b.iter(|| {
+        html! {
+            @for _ in 0..100 {
+                (text)
+            }
+        }
+    });
+}

(Posting here for future reference... if I ever find a way to make this reasonably fast in the worst case.)

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