Is this parser idiomatic and efficient? (probably not)

Hi there, I am fairly new to Rust and I wrote a simple program that loads in memory a bunch of small gzipped json files in order to extracts some stats.

Out of sheer curiosity, I wrote a similar program in Go and noticed that the execution time on few GBs of data was similar. This was before I had declared the json structure in Rust. Ouch!

My questions is if the code is idiomatic and how I can improve it further for performance. For now, I do not want to tinker with parallel programming.

I was considering switching to zune-inflate and simd_json, in order to reduce the time for both decompression and parsing. What do you think?

Any feedback is very welcome. Thank you!

There isn't any universally right answer on "how to improve performance", except to always benchmark, profile and apply optimizations where needed (and also to always use --release). Since it sounds like just loading the files to memory takes about the same time as both that and parsing in Go, I'd assume the first part is the bottleneck here so either a. this task is IO bound and you are unlikely to see performance improvements on your hardware, or b. the you may be able to improve it slightly by switching to a different deflate implementation. Either way you would have to try and measure this yourself.

2 Likes

Thank you for the answer, I will try to profile the code.

I need some help to make some sense out of the results I am getting.
As I was mentioning, I wanted to check the performance improvement of using zune_inflate instead of flate2.
It turns out that the results with the new library are slower! Can you tell me why? The results were consistent after multiple runs.

// zune_inflate -> File decompression time: 159607ms
    let open_file_start = Instant::now();
    let file = fs::read(file_path).unwrap();
    let mut decoder = DeflateDecoder::new(&file);
    let decompressed_data = decoder.decode_gzip().unwrap();
    let json_string = str::from_utf8(&decompressed_data).unwrap();
    let open_file_end = open_file_start.elapsed().as_millis();
[...]

// flate2 -> File decompression time: 117897ms
let open_file_start = Instant::now();
let file = File::open(file_path).expect("Failed to open file");
let mut decoder = GzDecoder::new(file);
let mut json_string = String::new();
    decoder
        .read_to_string(&mut json_string)
        .expect("Failed to read file");
    let open_file_end = open_file_start.elapsed().as_millis();
[...]

Why did you assume zune would be faster? looking at benchmarks, it seems inconsistent at best.

That's exactly my point about optimizing. It's almost impossible to predict the impact some change is going to have, it depends on your usecases, hardware etc. you just have to try it, and use profiling to guide you in choosing which parts to optimize.

1 Like

First, since you're caring about performance, you need to set up benchmarking. I highly recommend using Criterion for this, firstly because it generates awesome reports and makes it easy to profile your code, and secondly because it can compare results to previous runs and tell you if you've made a statistically significant change, or if the change is not clearly an improvement.

One big thing I can see is that you're showing quite big numbers for decompression time (which implies big files), but you're decompressing the whole file into memory, then parsing the data. I would expect to see a reduction in elapsed time if you instead used streaming I/O; put a BufRead around the GzDecoder, and use serde_json::from_reader instead of from_str.

There's a few minor things where you're using unidiomatic ways of doing things; these might be hurting performance, but might not:

  1. You turn your file names into String and &str in fn get_gzip_archives. I would expect paths to be handled as PathBuf or &Path instead. This is just about idiomatic Rust; I'd also expect to see you use things like extension() instead of ends_with() as you make this change.
  2. There's for loops where I'd expect to see iterator methods. I'd normally expect to see collect() instead of the for loop inside get_gzip_archives, and I'd expect to see the let bucket_name = in get_file_stats implemented via filter_map instead of returning an empty string if the name isn't present. In this case, there's a chance that the compiler will do better with iterator methods instead of a for loop (but benchmark to confirm/disprove this). Related, I'd not expect you to use String::new() if a value is unavailable - model this with Option<_> instead.
  3. There's a couple of places where you clone() something, but then don't hang onto ownership, where I'd expect to see you using borrowing in idiomatic Rust. In the code you've shared, I'd not expect to see clone() at all - there's no place where you need to copy things instead of borrowing. This is likely to impact performance, since you're making a copy that you don't actually need, but it's outside the sections you're timing.
1 Like