Skip to content

Optimize Write::write_fmt for Vec<u8>/Sink #37850

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

Closed
wants to merge 2 commits into from

Conversation

sinkuu
Copy link
Contributor

@sinkuu sinkuu commented Nov 18, 2016

A small optimization for formatted writing to Vec<u8>. This doesn't fix the issue, but benchmarks for #10761 (comment) :

 name                default ns/iter  override ns/iter  diff ns/iter   diff % 
 bench_write_macro1  22,253           17,460                  -4,793  -21.54% 
 bench_write_macro2  33,725           29,363                  -4,362  -12.93% 
 bench_write_ref     2,303            2,228                      -75   -3.26% 
 bench_write_value   2,263            2,252                      -11   -0.49%

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sinkuu sinkuu closed this Nov 18, 2016
@sinkuu sinkuu reopened this Nov 18, 2016
@sinkuu sinkuu changed the title Optimize Write::write_fmt for Vec<u8> Optimize Write::write_fmt for Vec<u8>/Sink Nov 18, 2016
@bluss
Copy link
Member

bluss commented Nov 18, 2016

Do you know roughly why this is an improvement?

@alexcrichton
Copy link
Member

Hm yeah I'm also a little baffled on how this could result in a speedup. The #[inline] tags on Sink make sense to me, but AFAIK the specializations of write_fmt should be what's generated in the standard case anyway...

@sinkuu
Copy link
Contributor Author

sinkuu commented Nov 18, 2016

Looking at IR diff, the significant difference looks like write_fmt beging inlined. But, just putting #[inline] on default write_fmt did't improve performance IIRC (I'll try it again).

https://gist.github.com/sinkuu/9f6c5e34e05c088e84c7330e2b146585

Tried: #[inline} on default write_fmt doesn't improve performance...

 name                old ns/iter  inline ns/iter  diff ns/iter  diff % 
 bench_write_macro1  24,012       24,130                   118   0.49% 
 bench_write_macro2  35,795       36,072                   277   0.77% 
 bench_write_ref     2,374        2,418                     44   1.85% 
 bench_write_value   2,410        2,290                   -120  -4.98% 

@bluss
Copy link
Member

bluss commented Nov 18, 2016

It's a very small program (the benchmark runner), so whole program optimization effects could cause this.

@sfackler
Copy link
Member

Why do we particularly care about the performance of Sink?

@sinkuu
Copy link
Contributor Author

sinkuu commented Nov 19, 2016

I tried to remove unnecessary error handling in write_fmt, and thought that this can be applied to Vec<u8> and Sink.

Since the optimizer seems to be able to do that (and the cause of performance gain was inlining), this PR may not make much sense.

@sinkuu sinkuu closed this Nov 19, 2016
@sinkuu sinkuu deleted the vec_write_fmt branch July 8, 2019 12:01
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.

6 participants