-
Notifications
You must be signed in to change notification settings - Fork 337
Faster JSON marshaling (and easyjson support) #112
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ import ( | |
| "regexp" | ||
| "sort" | ||
| "strings" | ||
|
|
||
| "github.com/mailru/easyjson/jwriter" | ||
| ) | ||
|
|
||
| var ( | ||
|
|
@@ -87,6 +89,29 @@ func (m Metric) FastFingerprint() Fingerprint { | |
| return LabelSet(m).FastFingerprint() | ||
| } | ||
|
|
||
| func (m Metric) MarshalEasyJSON(w *jwriter.Writer) { | ||
| w.RawByte('{') | ||
| first := true | ||
| for k, v := range m { | ||
| if !first { | ||
| w.RawByte(',') | ||
| } else { | ||
| first = false | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: be consistent how you handle the if-else here and in the LabelSet marshaling method (I guess I prefer the if-else variant, since it only assigns the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, I switched to the else, since it seems better :) (tests don't show much of a difference, but it looks better at least) |
||
| } | ||
| w.RawString(`"` + string(k) + `"`) | ||
| w.RawByte(':') | ||
| w.RawString(`"` + string(v) + `"`) | ||
| } | ||
| w.RawByte('}') | ||
| } | ||
|
|
||
| // MarshalJSON implements the json.Marshaler interface. | ||
| func (m Metric) MarshalJSON() ([]byte, error) { | ||
| w := jwriter.Writer{} | ||
| m.MarshalEasyJSON(&w) | ||
| return w.Buffer.BuildBytes(), w.Error | ||
| } | ||
|
|
||
| // IsValidMetricName returns true iff name matches the pattern of MetricNameRE. | ||
| // This function, however, does not use MetricNameRE for the check but a much | ||
| // faster hardcoded implementation. | ||
|
|
||
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.
I don't think this is an appropriate dependency to have in this package. Data models should just be data models.
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.
@brian-brazil this package already has a dep on encoding/json meaning marshaling is already a part of the model. Similarly custom marshal and unmarshal methods already exist in this package (and even in that file).
In golang you can't define methods on a struct outside of the package where the struct is defined, meaning that if you want a struct to adhere to an interface it must define the methods in the same package. We could move the marshal methods into a different file in the same package, but the existing ones are in the same files-- but if separate files in the package is preferred I'd be more than happy to move them around.