jmcnamara / rust_xlsxwriter

A Rust library for creating Excel XLSX files.
https://crates.io/crates/rust_xlsxwriter
Apache License 2.0
316 stars 25 forks source link

Refactor `xmlwriter.rs` #25

Closed adriandelgado closed 1 year ago

adriandelgado commented 1 year ago
jmcnamara commented 1 year ago

Could you add a before and after performance delta from hyperfine.

adriandelgado commented 1 year ago

image Not much difference. But this is my first step to ease some bigger changes later. Also, I think these functions are not changed very often.

adriandelgado commented 1 year ago

The perf. benefits of this PR get more noticeable after some of the other proposed changes. The change from &Vec<(&str, String)> to &[(&str, String)] is the most important because it unlocks more possibilities without any inconveniences.

jmcnamara commented 1 year ago

The change from &Vec<(&str, String)> to &[(&str, String)] is the most important because it unlocks more possibilities without any inconveniences.

Yes. It should probably have had a &[] signature regardless of anything else.

adriandelgado commented 1 year ago

Oops. The first force-push had my notes haha. Please ignore those.

jmcnamara commented 1 year ago

I still don't like the write_all() part of this change and don't see any improvement outside the margin of error so I'd prefer if you dropped that part of the PR and just restrict it to the changes in the reset() and escape_string() functions.

This question and the link in the answer suggests that write() and write_all() are the same in this context: https://users.rust-lang.org/t/io-write-write-vs-io-write-write-all/6341

Don't update the test section either. I want some of the tests to use vec() and I will fix the to_owned() as part of a general cleanup.

adriandelgado commented 1 year ago

This question and the link in the answer suggests that write() and write_all() are the same in this context

Yes, they are the same. However the write! macro uses write_fmt which need to pass through the formatter which has more complex machinery. In this case, I didn't saw a measurable difference though so I removed those changes. It probably gets optimized away by the compiler.

jmcnamara commented 1 year ago

Merged. Thanks.

I also added the vec! to [] change to main. If you don't have any other performance changes planned in the short term I'll publish these changes in a updated version and get back to the general porting.

adriandelgado commented 1 year ago

No relevant perf. changes left for now. I'm thinking of helping porting some features, too. For example, we need the set_labels_percentage at work.

jmcnamara commented 1 year ago

"Help" on porting usually just gets in my way but I'm happy to port that feature next.