Add optimizations to encoder and decoder #387
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guelfey
reviewed
Sep 21, 2024
There was a problem hiding this comment.
Thanks for your initiative, but this won't be merged without some larger changes / issues:
- For the actual performance improvements, please provide benchmarks and comparisons using
benchstat - You're combining lots of changes which are not strictly related into one single PR (e.g. changing the linting config and fixing the resulting issues). These might be valid, but should be discussed and merged separately to make review easier. Right now this is too big to review sensibly - for that reason, I only briefly scanned the PR and combined other issues I could spot easily that prevent merging.
| @@ -44,6 +43,7 @@ func (e InvalidTypeError) Error() string { | |||
| // their elements don't match. | |||
| func Store(src []interface{}, dest ...interface{}) error { | |||
| if len(src) != len(dest) { | |||
| fmt.Printf("%#v %#v\n", src, dest) | |||
| @@ -1,5 +1,5 @@ | |||
| module github.com/godbus/dbus/v5 | |||
| module github.com/pboyd04/dbus/v5 | |||
| "testing" | ||
| ) | ||
|
|
||
| /* This seems to be broken. Commenting out for now. |
There was a problem hiding this comment.
Do not just comment out tests. This test passes locally on my machine and in CI; it of course depends on having D-Bus installed. If it doesn't pass for some reason on your machine, please open a separate issue.
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| expected := [][]byte{ |
There was a problem hiding this comment.
these binary outputs should be moved to testdata files that are read during test runtime
|
Ok I will do this a bit more piecemeal. Here is the first of the new pulls only doing test changes so that all Benchmarks and tests are apples to apples. #394 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add optimizations to encoder and decoder to reduce allocations and increase speed