Skip to content

Conversation

@mxinden
Copy link
Contributor

@mxinden mxinden commented May 27, 2020

This pull request addresses the low-hanging-fruits within src/encoder/text.rs with the following commits:

  • src/benches&src/bin: Add basic text encoder benchmark

  • src/encoder/text: Use write_all instead of write_fmt

    Instead of going through write! which calls write_fmt use write_all directly skipping the additional formatting overhead.

  • src/encoder/text: Use regex crate to determine if escaping is needed

    Within escape_string instead of always allocating a new String and iterating through each character one-by-one, first check whether the input string needs any escaping, if not, return without any allocation. If it does need escaping, start from the first occurence.

Benchmark results on a Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz:

$ cargo benchcmp master optimized
 name                                 master ns/iter  optimized ns/iter  diff ns/iter   diff %  speedup 
 bench_text_encoder_with_escaping     131,884,199     105,675,740         -26,208,459  -19.87%   x 1.25 
 bench_text_encoder_without_escaping  117,592,719     73,070,167          -44,522,552  -37.86%   x 1.61

CPU flamegraph before:
master

CPU flamegraph after:
optimized

Memory allocation flamegraph before:
image

Memory allocation flamegraph after:
image

mxinden added 3 commits May 27, 2020 09:27
Instead of going through `write!` which calls `write_fmt` use
`write_all` directly skipping the additional formatting overhead.

Signed-off-by: Max Inden <[email protected]>
Within `escape_string` instead of always allocating a new String and
iterating through each character one-by-one, first check whether the
input string needs any escaping, if not, return without any allocation.
If it does need escaping, start from the first occurence.

Signed-off-by: Max Inden <[email protected]>
@mxinden
Copy link
Contributor Author

mxinden commented May 28, 2020

@lucab @kennytm would you mind giving this another review?

@lucab
Copy link
Member

lucab commented May 30, 2020

LGTM, but leaving some room for others' feedback.

@breezewish
Copy link
Member

It's interesting that write! is not efficient as I thought. @kennytm @nrc PTAL

@kennytm
Copy link

kennytm commented Jun 2, 2020

@breeswish yes std::fmt is not efficient because of the virtual calls, which is why packages like ufmt exists.

@mxinden
Copy link
Contributor Author

mxinden commented Jun 9, 2020

Any further comments on this patch?

@breeswish in case there are no objections, do you want me to merge my own pull-request, or rather always have someone other than the author click the merge button?

@mxinden
Copy link
Contributor Author

mxinden commented Jun 18, 2020

I am guessing there are no objections to this patch. @lucab @breeswish could one of you approve this pull request?

@lucab
Copy link
Member

lucab commented Jun 19, 2020

@mxinden stamped I think you can rebase-squash a couple of commits from this branch though. Feel free to self-merge when done.

@mxinden mxinden merged commit ab1ca72 into tikv:master Jun 19, 2020
@breezewish
Copy link
Member

@lucab Feel free to just merge PRs :)

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.

4 participants