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 structs that impl Display for escaping #881

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ollpu
Copy link
Collaborator

@ollpu ollpu commented Apr 22, 2024

Stacked on top of #880

As discussed in #870 (comment), instead of

escape_html(&mut writer, text);

write

write!(writer, "{}", EscapedHtml(text));

At the same time we can make StrWrite and friends private and less complicated.

I haven't evaluated the performance implications of this yet.

@Martin1887
Copy link
Collaborator

Looks very good and smart to me! But I think we need to test the performance penalty before merging it. Thanks!

@ollpu
Copy link
Collaborator Author

ollpu commented Apr 24, 2024

Not so great:

cargo bench
crdt_total              time:   [131.44 µs 131.50 µs 131.58 µs]
                        change: [+4.0821% +4.1774% +4.2607%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
  5 (5.00%) high mild
  3 (3.00%) high severe

crdt_html               time:   [50.832 µs 50.851 µs 50.870 µs]
                        change: [+14.604% +14.823% +15.036%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
  5 (5.00%) high mild
  3 (3.00%) high severe

crdt_all_options_parse  time:   [107.51 µs 107.55 µs 107.59 µs]
                        change: [+2.2069% +2.2952% +2.3752%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

crdt_parse              time:   [88.568 µs 88.690 µs 88.835 µs]
                        change: [-1.1698% -0.5629% -0.1917%] (p = 0.02 < 0.05)
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe

smart_punctuation       time:   [872.05 ns 872.58 ns 873.11 ns]
                        change: [-3.6300% -3.5129% -3.4036%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild

links_n_emphasis        time:   [1.2686 µs 1.2692 µs 1.2698 µs]
                        change: [-2.4608% -2.3504% -2.2480%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

unescapes               time:   [4.4079 µs 4.4094 µs 4.4111 µs]
                        change: [+0.7463% +0.8114% +0.8848%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 9 outliers among 100 measurements (9.00%)
  6 (6.00%) high mild
  3 (3.00%) high severe

autolinks_n_html        time:   [1.6281 µs 1.6346 µs 1.6491 µs]
                        change: [-3.6629% -3.2513% -2.5614%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe

cargo bench --features simd
crdt_total              time:   [109.21 µs 109.27 µs 109.32 µs]
                        change: [+3.8755% +4.0736% +4.2547%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) low mild
  2 (2.00%) high mild
  2 (2.00%) high severe

crdt_html               time:   [38.696 µs 39.201 µs 39.622 µs]
                        change: [+17.250% +18.419% +19.686%] (p = 0.00 < 0.05)
                        Performance has regressed.

crdt_all_options_parse  time:   [94.254 µs 94.695 µs 95.282 µs]
                        change: [+1.3280% +1.5903% +1.9182%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  4 (4.00%) high mild
  5 (5.00%) high severe

crdt_parse              time:   [78.402 µs 78.460 µs 78.520 µs]
                        change: [-0.7182% -0.5799% -0.4425%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 7 outliers among 100 measurements (7.00%)
  2 (2.00%) low mild
  5 (5.00%) high mild

smart_punctuation       time:   [906.87 ns 907.30 ns 907.73 ns]
                        change: [+2.2129% +2.3546% +2.4982%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

links_n_emphasis        time:   [1.3646 µs 1.3659 µs 1.3677 µs]
                        change: [+0.4749% +0.6685% +0.9421%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  3 (3.00%) high mild
  6 (6.00%) high severe

unescapes               time:   [4.7764 µs 4.7776 µs 4.7788 µs]
                        change: [+1.6330% +1.6841% +1.7336%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
  1 (1.00%) low severe
  2 (2.00%) low mild
  5 (5.00%) high mild
  1 (1.00%) high severe

autolinks_n_html        time:   [1.7091 µs 1.7153 µs 1.7273 µs]
                        change: [-0.9870% -0.7915% -0.4940%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 9 outliers among 100 measurements (9.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  6 (6.00%) high severe

@Martin1887
Copy link
Collaborator

crtd_total and crdt_html looks too bad... Have those benches been compiled with the optimization flags in Cargo.toml? I don't think so because they are not in the 0.11 branch.

If this performance downgrade is not due the compilation flags, I think we should try to improve the performance of this approach before merging it because the performance regression is significant.

@Martin1887
Copy link
Collaborator

I have run the benchmarks with the Cargo.toml optimizations and the difference of performance hurts 😞 :

cargo bench
crdt_total              time:   [267.05 µs 267.42 µs 267.91 µs]
                        change: [+18.049% +18.136% +18.241%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
  2 (2.00%) high mild
  7 (7.00%) high severe

crdt_html               time:   [97.275 µs 97.308 µs 97.345 µs]
                        change: [+17.381% +17.530% +17.668%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
  6 (6.00%) high mild
  4 (4.00%) high severe

crdt_all_options_parse  time:   [225.13 µs 225.31 µs 225.56 µs]
                        change: [+16.989% +17.244% +17.451%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

crdt_parse              time:   [198.01 µs 198.43 µs 199.24 µs]
                        change: [+17.302% +17.700% +18.028%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 19 outliers among 100 measurements (19.00%)
  4 (4.00%) low severe
  5 (5.00%) low mild
  3 (3.00%) high mild
  7 (7.00%) high severe

smart_punctuation       time:   [1.6568 µs 1.6607 µs 1.6672 µs]
                        change: [+4.7981% +5.2080% +5.5433%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) high mild
  5 (5.00%) high severe

links_n_emphasis        time:   [2.2786 µs 2.2829 µs 2.2886 µs]
                        change: [+2.1132% +2.5259% +2.9040%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) high mild
  9 (9.00%) high severe

unescapes               time:   [7.3092 µs 7.3848 µs 7.4697 µs]
                        change: [+2.6863% +3.2635% +3.8276%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
  1 (1.00%) high mild
  14 (14.00%) high severe

autolinks_n_html        time:   [3.0157 µs 3.0189 µs 3.0232 µs]
                        change: [+5.4275% +5.8034% +6.0782%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
  3 (3.00%) high mild
  8 (8.00%) high severe
cargo bench --all-features
crdt_total              time:   [201.01 µs 201.21 µs 201.52 µs]
                        change: [+6.8731% +6.9886% +7.1038%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
  2 (2.00%) high mild
  5 (5.00%) high severe

crdt_html               time:   [76.339 µs 76.357 µs 76.376 µs]
                        change: [+28.286% +28.717% +29.100%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
  4 (4.00%) high mild
  5 (5.00%) high severe

crdt_all_options_parse  time:   [177.79 µs 178.14 µs 178.76 µs]
                        change: [-1.0443% -0.4188% +0.0965%] (p = 0.17 > 0.05)
                        No change in performance detected.
Found 14 outliers among 100 measurements (14.00%)
  6 (6.00%) low severe
  1 (1.00%) low mild
  2 (2.00%) high mild
  5 (5.00%) high severe

crdt_parse              time:   [146.35 µs 146.76 µs 147.47 µs]
                        change: [+1.0230% +1.1972% +1.4127%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 13 outliers among 100 measurements (13.00%)
  6 (6.00%) low severe
  1 (1.00%) low mild
  1 (1.00%) high mild
  5 (5.00%) high severe

smart_punctuation       time:   [1.6830 µs 1.6834 µs 1.6837 µs]
                        change: [+1.8682% +2.3031% +2.6376%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) high mild
  6 (6.00%) high severe

links_n_emphasis        time:   [2.3000 µs 2.3017 µs 2.3037 µs]
                        change: [+0.5134% +0.7322% +1.1485%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 15 outliers among 100 measurements (15.00%)
  2 (2.00%) high mild
  13 (13.00%) high severe

unescapes               time:   [7.6489 µs 7.6584 µs 7.6717 µs]
                        change: [+2.0050% +2.2402% +2.5636%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
  5 (5.00%) high mild
  7 (7.00%) high severe

autolinks_n_html        time:   [2.7526 µs 2.7533 µs 2.7543 µs]
                        change: [+0.1459% +0.2366% +0.3171%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 13 outliers among 100 measurements (13.00%)
  3 (3.00%) low severe
  1 (1.00%) low mild
  2 (2.00%) high mild
  7 (7.00%) high severe

@ollpu
Copy link
Collaborator Author

ollpu commented Apr 28, 2024

I guess it's pretty conclusive anyway but crdt_parse shouldn't call the escaping functions at all, right?

@Martin1887
Copy link
Collaborator

It has many outliers and maybe there are instruction cache misses? I don't know, I executed the benchmarks without executing other CPU-intensive tasks, but an additional benchmark would be desirable to verify the performance loss.

This was referenced May 13, 2024
@Martin1887 Martin1887 added this to the v0.12 milestone May 13, 2024
Base automatically changed from branch_0.11 to master May 15, 2024 20:44
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.

None yet

2 participants