Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added type match check to read functions #937
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
Added type match check to read functions #937
Changes from all commits
5dc7e21
d5008b2
c89e238
c9038f7
5d404ea
ec2f84e
f4a5449
7ee7c82
371a858
0eb0cfc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, there is an overhead of generating a
TypeReference
each time, and this change adds another overhead of checking.Therefore, after checking benchmarks, I will add a document to avoid using it in situations where performance is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If performance is issue.... would simple
try-catch
instead of this type check(this PR) have better performance?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I confirmed that there was already an 8% drop in throughput before the change.
https://github.com/k163377/read-value-benchmark/blob/master/reports/results.csv
The throughput is high enough to begin with, so I don't think the performance of this function will be an issue in basic use cases.
However, I am considering giving only comments for use cases that require extreme performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a little more consideration, I decided not to make a performance statement because it is unlikely that the overhead of this function will actually dominate.
371a858