encoding/mvt: stable marshalling#93
Merged
paulmach merged 2 commits intopaulmach:masterfrom Apr 2, 2022
Merged
Conversation
Owner
|
Hi, thanks for the contribution. LGTM. You could put a shared []string buffer on the On the performance thing. If you guys are "big fans" and okay with it, I am too :). I don't have enough context to really make the call. I like the idea of it being stable for better caching and comparison, as you mentioned. |
Contributor
Author
|
Ok I updated with 4408fe0 to do what I believe you were suggesting - let me know if this isn't what you had in mind. The new benchmark numbers are: Looks like improvements on ns, B, and allocs. Great suggestion! |
Owner
|
Thank you. |
This file contains hidden or 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
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.
Hi! We've been using the library and are big fans. I've noticed one opportunity to (potentially) improve things. I'm trying to take an MD5 hash of the marshaled bytes to use as a fingerprint that I can compare with what the client sends me to see if they match, and it doesn't work quite right.
Currently, encode_properties iterates over a map to encode. Since there are no guarantees on iteration order for go maps, the output can differ each time for the same input. In practice, I've found the iteration order to be different almost every time.
func TestStableMarshalling(t *testing.T) {
layers := NewLayers(loadGeoJSON(t, maptile.New(17896, 24449, 16)))
values := make(map[string]bool)
}
The above test will typically produce 100 different md5 values.
This change makes encode_properties stable by getting the keys to the map, sorting them, and iterating over them. The benchmark numbers are:
Unstable:
Stable:
I'm happy to go whatever route you'd prefer here. I understand you may not want to impact the perf since the stability hasn't mattered up to now. I can perhaps add a separate method like
MarshalStable?Much appreciated!