Skip to content

Use structs that impl Display for escaping#881

Closed
ollpu wants to merge 2 commits intomasterfrom
escape-display
Closed

Use structs that impl Display for escaping#881
ollpu wants to merge 2 commits intomasterfrom
escape-display

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 Martin1887 requested a review from notriddle April 23, 2024 07:12
@Martin1887
Copy link
Collaborator

Martin1887 commented Apr 23, 2024

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

Martin1887 commented Apr 25, 2024

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

Martin1887 commented Apr 28, 2024

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

Martin1887 commented Apr 29, 2024

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
Closed
Merged
@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
@ollpu
Copy link
Collaborator Author

ollpu commented Aug 15, 2024

I did some profiling on crdt.md, and the overhead is pretty apparent there too. (write_fmt calls not inlined, significant amount of time spent in plumbing such as fmt::Formatter::new)

image

Abandoning this PR for now. I will preserve the branch in my own fork.

@ollpu ollpu closed this Aug 15, 2024
@ollpu ollpu deleted the escape-display branch August 15, 2024 12:12
@ollpu ollpu mentioned this pull request Dec 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@notriddle notriddle Awaiting requested review from notriddle

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.12

Development

Successfully merging this pull request may close these issues.

2 participants

@ollpu @Martin1887