Transition from dynamic dispatch to generics #20
Conversation
|
What also leads me to believe the issue is within |
|
I'll take a look at it in the morning. My gut says that it's the state between runs for multi part reading of rss codes. |
If that's the case I'll have to figure out a way to re-incorporate keeping the state after reads from within the readers. Thanks for the extra eyes on it! |
|
I do have an alternative implementation as well for the readers which should allow for holding onto the states. Will test it out in the morning as well. |
There was a problem hiding this comment.
I find your method for converting the Binarizer trait especially interesting. I think this is where I got hung up when I attempted this, it didn't occur to me to put a type into the trait, so I had to make the trait generic, which spread a lot more generics parameters all over the code.
|
Ok, I've fully reviewed it, and I'm impressed with out cleanly this works. My attempts all resulted in the types getting rather bloated. I hadn't thought all the way through before, but it's interesting how differently the multi-line read works for RSS vs every other type that works similarly (multi-image-read for pdf417, for instance). The RSS reader code pretty clearly was contributed by someone other than the original devs (not surprising, given that the the pdf417 code was as well). As for benchmarking, I don't have a great way to approach it. I haven't built any benchmarks for the library, though there were some benchmark images included with the original which they used to test their client application. I'll look into what it takes to get those working and figure something out. While I get that worked out, I think this looks good to be part of our 0.4.0 release, I'd like to get the decoupled character encoding / decoding work into that as well. Excellent work, thank you! |
|
I can look into benchmarking as well. I think we can copy from the integration tests and make benchmark versions of them. That may be a big thing to implement so it may be better in its own PR after this were to be merged. Until then I'll play around with the idea |
|
I think it makes the interface so much cleaner that it might make sense even if the benchmarks aren't conclusive. |
|
Added some small benchmarks. I ran them locally against the current main branch and there is definitely some improvement for the more advanced formats (maxicode, qrcode). Almost nothing or just noise from my pc for more simple ones such as codabar. |
|
Nice! Thinking about where the (biggest) performance issues are, I think the GenericMultipleBarcodeReader will be an interesting one to benchmark. It can get very slow for larger images. |
|
I added this benchmark: fn multi_barcode_benchmark(c: &mut Criterion) {
let mut image = get_image("test_resources/blackbox/multi-1/1.png");
c.bench_function("multi_barcode", |b| {
b.iter( || {
let mut reader = GenericMultipleBarcodeReader::new(MultiFormatReader::default());
let _res = reader.decode_multiple(&mut image);
});
});
}I think criterion runs in release mode, though, so I just realized that your optimizations weren't included when I benched the main branch, that probably throws things off a little. Anyway, this is the result I got: I cannot fully explain the differences here, and I think I need to run them again in a more controlled way. |
|
Ok, I fully admit that I don't understand benchmarking very well. Without any modifications, I ran all my the benchmarks again and got the following: So, without changes to the codebase in any way besides rerunning I also don't know enough about the rust arm64 target vs the amd64 target to know how different I should expect runs between them to be. The only place where a difference in performance makes sense to me is in the |
|
I think the issue is how much noise is running on a PC and how at times it can be hit or miss. It's suggested to either increase the noise threshold (it's pretty small for default) and also increase the sample size cause 100 doesn't mean much if there are a handful of outliers. Since generics favor strict typing, one thing could be to create a variable inside the struct for each reader similar to what was done for the RSS ones. Then the creation wont happen on every iteration but it would slightly increase the size on the stack. Can still box them into another internal struct and it would only use a pointer if that were necessary. I think the main thing is almost that can be changed at this point would change performance in minimal ways. If there are any major differences then they'd be more easily visible with benchmarks. It's hard to tell because the change might be minimal or noise is throwing off even the average of the samples. Noise is just produced from literally any process running on the system even background tasks which is hard to avoid without an official benchmark machine |
|
Overall, it's hard to tell if dynamic dispatch really hurt performance. Everything is only abstracted over one trait I prefer the strict typing from generics for Rust but in the end if it isn't deemed necessary or unwarranted this PR can be scrapped or archived. I'll always have it on my fork if the idea were to ever see interest again. |
|
I ran more controlled benchmarks. MacOS makes that a little hard but I think I succeeded. For most cases it's pretty close. I think that the generics solution makes for a better api, so my plan is to merge this PR. My one concern is efficiency for bulk runs reusing a single MultiFormatReader many times, but I'm going to put an issue in to create a new implementation of it that is optimized for many runs. The startup cost and overall memory cost of the one you built seems better for most situations. I think that's a reasonable solution: keeping your implementation as the default and adding another "multi run optimized" reader for those edge cases it makes sense. |
|
That's a good idea. There is definitely decent overhead for bulk reads which could be reduced. I do have to fix one thing relating to resets that I forgot regarding the RSS readers so I'll commit that and then this PR should be all set |
|
All checks passed, I'm going to merge. |
This is a draft PR to transition core functionality to use generics.
I have not yet done any benchmarking to see if this actually improves performance.
Before benchmarking, I would like to make sure all tests are running properly. The internals of the multi readers has changed to properly implement this which seems to have caused issues for both RSS barcode formats. I am drafting this PR for now and will continue to investigate the cause.
If anyone else would like to take a look to find the issue, feel free! No core math has changed so my only guess is it's something within the
MultiFormatReaderorMultiFormatOneDReader.