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

Use encoding/json as JSON decoder instead of mapstructure #6680

Merged
merged 15 commits into from
Oct 29, 2019

Conversation

s-mang
Copy link
Contributor

@s-mang s-mang commented Oct 24, 2019

After research and benchmarking, I have determined that using the Go standard library's encoding/json as a JSON decoder instead of mitchellh/mapstructure will yield massive performance gains, particularly in the case where we are passing around large JSON values (benchmarks are showing ~300x speed improvement in this particular case).

In this PR, I have changed JSON decoding in our agent code to use only the standard library's encoding/json.

Users should see immediate performance improvements for most HTTP agent endpoints with a request body. Most endpoints should see a 1x-10x improvement in decoding time per request, regardless of the request body. K/V writes (Txn endpoint) should see O(100x) decoding time improvements for large Values.
Resource utilization for agent JSON decoding should also improve with this PR, ~same order of magnitude per endpoint.

Fixes #6147

@s-mang s-mang requested a review from a team October 24, 2019 21:27
@s-mang
Copy link
Contributor Author

s-mang commented Oct 28, 2019

Here are before/after benchmarks per endpoint. These values == $before / $after. Remember that small deviations in benchmarks can't be taken too seriously. Look for the large factors.

Note the last value in particular (the original bug).

  Small JSON All Vals 1KB Small except 1 val is 100KB
ACLPolicyWrite 4 1 0.5
ACLToken 5 1 0.5
ACLTokenClone 5 1 0.5
ACLRoleWrite 6.666666667 1 0.5
ACLBindingRuleWrite 3 1.5 1
ACLAuthMethodWrite 3 1.666666667 1
ACLLogin 3 1.5 1
ACLUpdate 5 1.666666667 1
AgentRegisterCheck 5 1 0.5
AgentCheckUpdate 5 2 1
AgentRegisterService 3.333333333 0.5 1
AgentToken 5 2 1
AgentConnectAuthorize 10 1 1
CatalogRegister 5 1 1
CatalogDeregister 5 1.5 1
ConnectCAConfigurationSet 5 4 1
CoordinateUpdate 4 2.5 1
DiscoveryChainRead 6 1.666666667 0.5
IntentionCreate 5 1 0.5
IntentionSpecificUpdate 3.333333333 1 0.5
OperatorKeyringEndpoint 10 2 1
OperatorAutopilotConfiguration 6 3.333333333 1
PreparedQueryGeneral_Create 5 2 1
PreparedQueryGeneral_Update 10 1 1
SessionCreate 4 0.5 0.5
TxnConvertOps 5 1 100

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is looking good. I left a handful of questions inline.

agent/http.go Outdated Show resolved Hide resolved
agent/structs/acl.go Show resolved Hide resolved
agent/structs/check_definition.go Outdated Show resolved Hide resolved
api/health.go Show resolved Hide resolved
api/operator_autopilot.go Show resolved Hide resolved
agent/structs/check_definition.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@s-mang s-mang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

agent/http.go Outdated Show resolved Hide resolved
agent/structs/acl.go Show resolved Hide resolved
agent/structs/check_definition.go Outdated Show resolved Hide resolved
agent/structs/check_definition.go Outdated Show resolved Hide resolved
api/health.go Show resolved Hide resolved
api/operator_autopilot.go Show resolved Hide resolved
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@s-mang s-mang merged commit 78ad820 into master Oct 29, 2019
@s-mang s-mang deleted the csl-109-unmarshal-methods branch October 29, 2019 18:22
@hanshasselberg
Copy link
Member

I am late to the party, but the numbers are exciting!!

@ghost
Copy link

ghost commented Jan 25, 2020

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

@ghost ghost locked and limited conversation to collaborators Jan 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance of mapstructure in HTTP API
3 participants