Skip to content
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

feat: decoder preallocate messages #493

Closed
wants to merge 1 commit into from

Conversation

muktihari
Copy link
Owner

@muktihari muktihari commented Oct 12, 2024

Add decode option to precalculate messages first before the actual decoding to avoid re-allocation when appending each decoded message. This is only eligible if the given io.Reader is an io.ReadSeeker and WithBroadcastOnly() is unset.

How fast it is may depends on I/O performance as we need to read the file twice, but typically, it beats the cost of re-allocation especially when the FIT file contains so many messages, e.g. more than 32k messages (I measure up to 237k messages from an ultra-cycling and ultra-run activities), the more the messages we have, the greater the benefit we'll gain. But more measurements are needed since users' cases may vary. I decide to not enable this by default since I'm not confident enough that this will cover most cases and probably leaving this as an opt-in feature is the best decision, we'll see.

This PR is open for feedback. Please feel free to share your thoughts.

@muktihari muktihari added enhancement New feature or request performance Changes related to performance improvement labels Oct 12, 2024
@muktihari muktihari self-assigned this Oct 12, 2024
@muktihari muktihari added this to the Unplanned milestone Oct 12, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2024

Codecov Report

Attention: Patch coverage is 8.69565% with 84 lines in your changes missing coverage. Please review.

Project coverage is 98.21%. Comparing base (078b1cc) to head (b5ab6d7).

Files with missing lines Patch % Lines
decoder/decoder.go 6.74% 82 Missing and 1 partial ⚠️
decoder/readbuffer.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master     #493      +/-   ##
===========================================
- Coverage   100.00%   98.21%   -1.79%     
===========================================
  Files           42       42              
  Lines         4618     4708      +90     
===========================================
+ Hits          4618     4624       +6     
- Misses           0       83      +83     
- Partials         0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@muktihari
Copy link
Owner Author

Test coverage will be added later once everything is settled.

@muktihari muktihari force-pushed the feat/decoder-preallocate-messages branch from 6229e53 to b5ab6d7 Compare October 19, 2024 04:17
@muktihari muktihari removed this from the Unplanned milestone Dec 19, 2024
@muktihari
Copy link
Owner Author

I will drop this since the benefits are considered small compared to the complexity it adds to our code. Additionally, having too many options will create a headache for users who are just starting to learn how to use this library. So let's keep it simple for now.

With our existing building blocks (Decoder, MesgListener, and RawDecoder), users have the ability to implement this approach on their own. However, they can also request this feature by providing a clear rationale for why it should be enabled.

@muktihari muktihari closed this Dec 19, 2024
@muktihari muktihari deleted the feat/decoder-preallocate-messages branch December 19, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request open for feedback performance Changes related to performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants