Switch from encoding/json -> jsoniter #570

jacksontj · 2019-05-08T03:11:17Z

Signed-off-by: Thomas Jackson jacksontj.89@gmail.com

Fixes #565

krasi-georgiev · 2019-05-08T22:05:38Z

do you think you can add some benchmarking so we can see what is the actual performance gains?

jacksontj · 2019-05-09T16:00:24Z

Fixed bug in benchmarks, for some reason the difference isn't as large as I expected:

$ go test -run=x -bench=. -benchmem
goos: linux
goarch: amd64
pkg: github.com/prometheus/client_golang/api/prometheus/v1
BenchmarkSerialization/10/10/unmarshal/encoding/json-8         	    5000	    258376 ns/op	   25970 B/op	     564 allocs/op
BenchmarkSerialization/10/10/unmarshal/jsoniter-8              	   10000	    231181 ns/op	   32649 B/op	     850 allocs/op
BenchmarkSerialization/10/100/unmarshal/encoding/json-8        	     500	   2320385 ns/op	  256520 B/op	    5964 allocs/op
BenchmarkSerialization/10/100/unmarshal/jsoniter-8             	    1000	   2187491 ns/op	  327974 B/op	    8950 allocs/op
BenchmarkSerialization/10/1000/unmarshal/encoding/json-8       	      50	  23265065 ns/op	 2571482 B/op	   59970 allocs/op
BenchmarkSerialization/10/1000/unmarshal/jsoniter-8            	      50	  22021770 ns/op	 3287473 B/op	   89955 allocs/op
BenchmarkSerialization/100/10/unmarshal/encoding/json-8        	     500	   2568609 ns/op	  258018 B/op	    5606 allocs/op
BenchmarkSerialization/100/10/unmarshal/jsoniter-8             	    1000	   2239641 ns/op	  326580 B/op	    8501 allocs/op
BenchmarkSerialization/100/100/unmarshal/encoding/json-8       	      50	  23747338 ns/op	 2576962 B/op	   59650 allocs/op
BenchmarkSerialization/100/100/unmarshal/jsoniter-8            	      50	  22705622 ns/op	 3288373 B/op	   89526 allocs/op
BenchmarkSerialization/100/1000/unmarshal/encoding/json-8      	       5	 233949527 ns/op	26711548 B/op	  600269 allocs/op
BenchmarkSerialization/100/1000/unmarshal/jsoniter-8           	       5	 228011137 ns/op	33462769 B/op	  899805 allocs/op
BenchmarkSerialization/1000/10/unmarshal/encoding/json-8       	      50	  26338162 ns/op	 2596940 B/op	   56224 allocs/op
BenchmarkSerialization/1000/10/unmarshal/jsoniter-8            	      50	  23486559 ns/op	 3282834 B/op	   85182 allocs/op
BenchmarkSerialization/1000/100/unmarshal/encoding/json-8      	       5	 241223130 ns/op	27157049 B/op	  678011 allocs/op
BenchmarkSerialization/1000/100/unmarshal/jsoniter-8           	       5	 227933347 ns/op	33707764 B/op	  974806 allocs/op
BenchmarkSerialization/1000/1000/unmarshal/encoding/json-8     	       1	2324619810 ns/op	311494832 B/op	 6319035 allocs/op
BenchmarkSerialization/1000/1000/unmarshal/jsoniter-8          	       1	2155111994 ns/op	361141288 B/op	 9300019 allocs/op
PASS
ok  	github.com/prometheus/client_golang/api/prometheus/v1	34.515s

I'll need to spend some more time on this later :/

krasi-georgiev · 2019-05-14T08:22:13Z

any progress with this?

jacksontj · 2019-05-15T05:12:03Z

Some, at this point I can confirm that the benchmarks are accurate (meaning the perf improvement isn't what I expected). So this will require more investigation, but its possible changes will be required elsewhere as well (e.g. common package). I'll try to get some more time to investigate this week.

jacksontj · 2019-05-15T05:22:30Z

I've also added some marshal benchmarks (for anyone else interested):

$ go test -run=x -bench=. -benchmem
goos: linux
goarch: amd64
pkg: github.com/prometheus/client_golang/api/prometheus/v1
BenchmarkSerialization/10/10/marshal/encoding/json-8         	    3000	    398908 ns/op	   55704 B/op	    1972 allocs/op
BenchmarkSerialization/10/10/marshal/jsoniter-8              	    3000	    432072 ns/op	   56143 B/op	    1932 allocs/op
BenchmarkSerialization/10/10/unmarshal/encoding/json-8       	    5000	    274671 ns/op	   26698 B/op	     764 allocs/op
BenchmarkSerialization/10/10/unmarshal/jsoniter-8            	    5000	    237565 ns/op	   33371 B/op	    1050 allocs/op
BenchmarkSerialization/10/100/marshal/encoding/json-8        	     300	   3930002 ns/op	  549498 B/op	   19975 allocs/op
BenchmarkSerialization/10/100/marshal/jsoniter-8             	     300	   4278254 ns/op	  554065 B/op	   19934 allocs/op
BenchmarkSerialization/10/100/unmarshal/encoding/json-8      	    1000	   2325803 ns/op	  256444 B/op	    5964 allocs/op
BenchmarkSerialization/10/100/unmarshal/jsoniter-8           	     500	   2332108 ns/op	  328020 B/op	    8950 allocs/op
BenchmarkSerialization/10/1000/marshal/encoding/json-8       	      30	  41352309 ns/op	 6008460 B/op	  200016 allocs/op
BenchmarkSerialization/10/1000/marshal/jsoniter-8            	      30	  46626714 ns/op	 5561751 B/op	  199967 allocs/op
BenchmarkSerialization/10/1000/unmarshal/encoding/json-8     	      50	  24766228 ns/op	 2571484 B/op	   59970 allocs/op
BenchmarkSerialization/10/1000/unmarshal/jsoniter-8          	      50	  23100204 ns/op	 3287492 B/op	   89955 allocs/op
BenchmarkSerialization/100/10/marshal/encoding/json-8        	     300	   4334550 ns/op	  570430 B/op	   19706 allocs/op
BenchmarkSerialization/100/10/marshal/jsoniter-8             	     300	   4557554 ns/op	  561875 B/op	   19305 allocs/op
BenchmarkSerialization/100/10/unmarshal/encoding/json-8      	     500	   2861914 ns/op	  265226 B/op	    7606 allocs/op
BenchmarkSerialization/100/10/unmarshal/jsoniter-8           	     500	   2473652 ns/op	  333875 B/op	   10502 allocs/op
BenchmarkSerialization/100/100/marshal/encoding/json-8       	      30	  43987262 ns/op	 6039315 B/op	  199747 allocs/op
BenchmarkSerialization/100/100/marshal/jsoniter-8            	      30	  46285685 ns/op	 5558747 B/op	  199339 allocs/op
BenchmarkSerialization/100/100/unmarshal/encoding/json-8     	      50	  26428125 ns/op	 2576977 B/op	   59650 allocs/op
BenchmarkSerialization/100/100/unmarshal/jsoniter-8          	     100	  23110238 ns/op	 3283907 B/op	   89514 allocs/op
BenchmarkSerialization/100/1000/marshal/encoding/json-8      	       3	 409665279 ns/op	59081770 B/op	 1999900 allocs/op
BenchmarkSerialization/100/1000/marshal/jsoniter-8           	       3	 430603410 ns/op	58996882 B/op	 1999499 allocs/op
BenchmarkSerialization/100/1000/unmarshal/encoding/json-8    	       5	 233062181 ns/op	26713731 B/op	  658270 allocs/op
BenchmarkSerialization/100/1000/unmarshal/jsoniter-8         	       5	 229206061 ns/op	33464878 B/op	  957805 allocs/op
BenchmarkSerialization/1000/10/marshal/encoding/json-8       	      30	  44881487 ns/op	 6123904 B/op	  197052 allocs/op
BenchmarkSerialization/1000/10/marshal/jsoniter-8            	      30	  45518786 ns/op	 5661321 B/op	  193044 allocs/op
BenchmarkSerialization/1000/10/unmarshal/encoding/json-8     	      50	  26742119 ns/op	 2596950 B/op	   56224 allocs/op
BenchmarkSerialization/1000/10/unmarshal/jsoniter-8          	      50	  23756123 ns/op	 3282841 B/op	   85182 allocs/op
BenchmarkSerialization/1000/100/marshal/encoding/json-8      	       3	 388438656 ns/op	57343797 B/op	 1997168 allocs/op
BenchmarkSerialization/1000/100/marshal/jsoniter-8           	       3	 434228624 ns/op	59020432 B/op	 1993193 allocs/op
BenchmarkSerialization/1000/100/unmarshal/encoding/json-8    	       5	 234770845 ns/op	27129120 B/op	  600611 allocs/op
BenchmarkSerialization/1000/100/unmarshal/jsoniter-8         	       5	 220862249 ns/op	33679800 B/op	  897405 allocs/op
BenchmarkSerialization/1000/1000/marshal/encoding/json-8     	       1	3830350089 ns/op	623496024 B/op	19997237 allocs/op
BenchmarkSerialization/1000/1000/marshal/jsoniter-8          	       1	4229907983 ns/op	689501776 B/op	19993300 allocs/op
BenchmarkSerialization/1000/1000/unmarshal/encoding/json-8   	       1	2277582499 ns/op	311490592 B/op	 6199036 allocs/op
BenchmarkSerialization/1000/1000/unmarshal/jsoniter-8        	       1	2082645407 ns/op	361136920 B/op	 9180019 allocs/op
PASS
ok  	github.com/prometheus/client_golang/api/prometheus/v1	69.265s

Which seems to indicate that the improvements claims from prometheus/prometheus#3536 aren't accurate.

api/prometheus/v1/api_bench_test.go

brian-brazil · 2019-05-15T07:03:37Z

SamplePair is likely the issue here, I did a custom marshalling in Prometheus for Point. As a general note we're trying to get away from common/model.

jacksontj · 2019-05-15T15:15:35Z

I added Type Encoder/Decoder funcs and am now seeing the perf improvement I was expecting. This seems like a pretty ugly way to do this (since we have to copy/paste this marshal logic all over). So maybe it would be helpful to change the marshaling/unmarshaling in common to be fast?

benchmark                                                 old ns/op      new ns/op     delta
BenchmarkSerialization/1000/1000/marshal/jsoniter-8       4125222362     201316013     -95.12%
BenchmarkSerialization/1000/1000/unmarshal/jsoniter-8     2060620113     218847915     -89.38%

benchmark                                                 old allocs     new allocs     delta
BenchmarkSerialization/1000/1000/marshal/jsoniter-8       19993541       2030           -99.99%
BenchmarkSerialization/1000/1000/unmarshal/jsoniter-8     9240110        998003         -89.20%

benchmark                                                 old bytes     new bytes     delta
BenchmarkSerialization/1000/1000/marshal/jsoniter-8       689480872     106189225     -84.60%
BenchmarkSerialization/1000/1000/unmarshal/jsoniter-8     361144008     9749326       -97.30%

brian-brazil · 2019-05-15T15:57:05Z

It wouldn't make sense to add those to common (especially as we're trying to do away with model). What I'd suggest is more appropriate structures be defined here.

jacksontj · 2019-05-15T16:01:08Z

If thats the case, then I'd just add these here for now and leave the model move for a separate PR; this is a huge perf improvement that I'd like to get in sooner.

krasi-georgiev · 2019-05-16T13:18:43Z

now with this custom Marshaling/Unmarshaling for model.SamplePair, do you think it is a good idea to also add a test to ensure the output is what is expected given some generated input data.

api/prometheus/v1/api.go

api/prometheus/v1/api_bench_test.go

krasi-georgiev

Do you think it would make sense to add some benchmarking for model.Vector as well?

api/prometheus/v1/api_bench_test.go

jacksontj · 2019-05-16T14:49:06Z

Do you think it would make sense to add some benchmarking for model.Vector as well?

We could, but since the marshaling methods are on model.SamplePair we get more of them from Matrix than Vector. If we wanted to improve overall performance more we'd need to create these marshal methods for each of the types, which I'd prefer to leave for another PR (since this seemingly simple one is already over a week in ;) )

krasi-georgiev · 2019-05-16T14:56:08Z

We could, but since the marshaling methods are on model.SamplePair we get more of them from Matrix than Vector. If we wanted to improve overall performance more we'd need to create these marshal methods for each of the types, which I'd prefer to leave for another PR (since this seemingly simple one is already over a week in ;) )

Yep It makes sense. Let's leave for another PR.

now with this custom Marshaling/Unmarshaling for model.SamplePair, do you think it is a good idea to also add a test to ensure the output is what is expected given some generated input data.

how about that one?

jacksontj · 2019-05-16T15:21:04Z

I copied over the tests (basically) from prom's web api. When I did so I found that model.Time doesn't properly unmarshal negative times (which I didn't think were even allowed?). I have opened prometheus/common#193 to fix the unmarshaling, we can either (1) get that merged or (2) remove the test case with the negative time.

cc @krasi-georgiev

brian-brazil · 2019-05-16T15:41:58Z

(which I didn't think were even allowed?)

Times are int64s, so it is permitted though I've never come across it in practice.

api/prometheus/v1/api.go

krasi-georgiev · 2019-05-17T08:19:56Z

api/prometheus/v1/api_bench_test.go

+	return m
+}
+
+func BenchmarkSerialization(b *testing.B) {


I would prefer a more descriptive name like: BenchmarkSamplesSerialization or even BenchmarkSamplesJsonSerialization or BenchmarkSamplesJsonMarshaling

api/prometheus/v1/api_test.go

krasi-georgiev · 2019-05-17T08:42:40Z

@beorn7 any idea why is this test failing https://travis-ci.org/prometheus/client_golang/jobs/533629401
the PR seems unrelated to this failing test.

jacksontj · 2019-05-23T14:36:56Z

@beorn7 Fixed the imports, although it looks like the unmarshaling doesn't work for nan/inf in <1.11 of go; do we need to support those versions or are we okay to drop them?

beorn7 · 2019-05-23T15:36:29Z

We have a commitment to support the last three Go minor releases at least, more if possible without undue hassle. Thus, dropping 1.9 should be fine, but for Go 1.10, we should wait for 1.13 to be out for a while.

Not sure what's a good way out of this.

jacksontj · 2019-05-23T15:49:39Z

@beorn7 I have checked through the release notes and I don't see why it would be fixed in that version. I have also downloaded 1.10 and run the tests locally using it and they pass, so I'm not sure why its failing in CI

$ GO111MODULE= /usr/local/go.1.10/bin/go test -short  ./...
ok  	github.com/prometheus/client_golang/api	0.006s
ok  	github.com/prometheus/client_golang/api/prometheus/v1	0.004s

beorn7 · 2019-05-23T23:24:57Z

Semi-educated guess: This has to do with Go Modules support in Go 1.11+, but not in Go 1.10 and earlier.

With Go Modules, the tests use Prometheus 2.9. Without Go Modules, they use the tip of master, which has changed the encoding of the alert values. We need #585 merged to be compatible again, but we will only do that once Prometheus 2.10 is released and tagged.

jacksontj · 2019-05-23T23:35:24Z

@beorn7 I don't think so? The error is marshaling a model.SamplePair which should have nothing to do with the alert value. From some more looking it seems that this is actually due to jsoniter upstream (json-iterator/go#365) -- apparently that marshaling output is against RFC, but from what I understand that is what prometheus is expecting?

beorn7 · 2019-05-24T09:22:26Z

Thanks for the investigation. Indeed, my semi-educated guess was way too semi…

My guess was right, however, about Go Modules support being the issue. Go 1.11+ take json-iterator/go at the tag v1.1.6, while the earlier Go versions take the head of master and thus include the fix.

The encoding of Inf and NaN is indeed required by Prometheus. How could that have ever worked?

beorn7 · 2019-05-24T09:26:34Z

Early result: The Prometheus server is not using model.SamplePair to marshal sample pairs into JSON. The sample value is represented as a string.

See https://prometheus.io/docs/prometheus/latest/querying/api/#expression-query-result-formats: “JSON does not support special float values such as NaN, Inf, and -Inf, so sample values are transferred as quoted JSON strings rather than raw numbers.”

jacksontj · 2019-05-24T14:08:14Z

So should we change model SamplePair to a string instead of a float64?

…

On Fri, May 24, 2019, 2:26 AM Björn Rabenstein ***@***.***> wrote: Early result: The Prometheus server is *not* using model.SamplePair to marshal sample pairs into JSON. The sample value is represented as a string. See https://prometheus.io/docs/prometheus/latest/querying/api/#expression-query-result-formats: “JSON does not support special float values such as NaN, Inf, and -Inf, so sample values are transferred as quoted JSON strings rather than raw numbers.” — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#570?email_source=notifications&email_token=AAMLPHPYCOTLZCBW4XWFWF3PW6YE3A5CNFSM4HLOALJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWEVNYA#issuecomment-495539936>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMLPHJMDO4HVABXCRKK7KLPW6YE3ANCNFSM4HLOALJA> .

beorn7 · 2019-05-24T14:25:18Z

model.SamplePair is correct, a value in Prometheus is a float64.
However, it's not suitable for JSON marshaling, as JSON floats are too restricted. (AFAIK, it is used nowhere for JSON marshalling.)

I'm not an expert when it comes to JSON APIs and testing thereof. Perhaps just provide the actual JSON code as reference for the test? You could also check out the prometheus/prometheus code to see how the JSON is created in the actual Prometheus API server code (which I'm not very familiar with). Just random ideas… Perhaps @krasi-georgiev has some advice.

krasi-georgiev · 2019-05-24T23:57:37Z

will check in few days.

jacksontj · 2019-05-25T00:11:46Z

@beorn7 @krasi-georgiev I have updated the PR to support this. Since we are already implementing our own marshal, I'm just doing the write as bytes instead of as a float64 -- this version works on jsoniter master.

beorn7 · 2019-05-25T11:55:26Z

Is there trailing whitespace in ./api/prometheus/v1/api.go L94?

beorn7 · 2019-05-25T11:56:02Z

@krasi-georgiev besides the formatting nit, does this look good to you?

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com> Fixes prometheus#565

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

jacksontj · 2019-05-25T15:08:30Z

ran go fmt (fixed the whitespace), the CI run should pass now.

jacksontj · 2019-05-25T15:13:53Z

And that test failure I've seen before that test is flaky for some reason :/ I don't see how I can re-trigger the tests -- I assume a maintainer has to do that?

beorn7 · 2019-05-25T15:27:08Z

Restarted the flaky test. (I have #573 in my pipeline to unflake.)

jacksontj · 2019-05-25T17:08:41Z

@beorn7 tests have passed, so we should be good for a merge

beorn7 · 2019-05-25T17:45:40Z

I'll just wait for 👍 from @krasi-georgiev as he is the maintainer of this code.

krasi-georgiev · 2019-05-26T11:08:49Z

back to full speed work mode in 1-2 days so will approve soon.

jacksontj force-pushed the issue_565 branch 2 times, most recently from f00f2c9 to ef34012 Compare May 8, 2019 14:34

beorn7 requested a review from krasi-georgiev May 8, 2019 21:35

jacksontj force-pushed the issue_565 branch from ae49a4a to 8625c84 Compare May 9, 2019 15:59

jacksontj mentioned this pull request May 15, 2019

Switch to json-iterator for v1 api. prometheus/prometheus#3536

Merged

brian-brazil reviewed May 15, 2019

View reviewed changes

api/prometheus/v1/api_bench_test.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed May 16, 2019

View reviewed changes

api/prometheus/v1/api.go Outdated Show resolved Hide resolved

api/prometheus/v1/api.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed May 16, 2019

View reviewed changes

api/prometheus/v1/api_bench_test.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed May 16, 2019

View reviewed changes

api/prometheus/v1/api_bench_test.go Outdated Show resolved Hide resolved

jacksontj force-pushed the issue_565 branch from ecd7a2e to 2321412 Compare May 16, 2019 15:17

jacksontj mentioned this pull request May 16, 2019

Fix unmarshal of negative times prometheus/common#193

Merged

jacksontj force-pushed the issue_565 branch from 2321412 to 2c03b23 Compare May 16, 2019 15:25

jacksontj force-pushed the issue_565 branch 2 times, most recently from 2bfd73b to f8f16b7 Compare May 17, 2019 04:14

krasi-georgiev reviewed May 17, 2019

View reviewed changes

jacksontj force-pushed the issue_565 branch from 0bfc6ae to 2589011 Compare May 23, 2019 14:26

jacksontj force-pushed the issue_565 branch 2 times, most recently from 080bbdd to 9299023 Compare May 25, 2019 00:10

jacksontj added 2 commits May 25, 2019 08:07

Switch from encoding/json -> jsoniter

b47e76a

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com> Fixes prometheus#565

Add benchmark for (un)marshal

08244dc

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

jacksontj force-pushed the issue_565 branch from 9299023 to 08244dc Compare May 25, 2019 15:07

beorn7 approved these changes May 25, 2019

View reviewed changes

krasi-georgiev merged commit 7e60220 into prometheus:master May 28, 2019

mweissbacher mentioned this pull request Jul 12, 2019

Adding prometheus host to listen on to config spiffe/spire#976

Merged

This was referenced Sep 16, 2019

errors in querier: []promql.Point: unsupported value: NaN cortexproject/cortex#1668

Closed

vendor: Update json-iterator prometheus/prometheus#6030

Merged

Switch from encoding/json -> jsoniter #570

Switch from encoding/json -> jsoniter #570

Conversation

jacksontj commented May 8, 2019 • edited

krasi-georgiev commented May 8, 2019

jacksontj commented May 9, 2019

krasi-georgiev commented May 14, 2019

jacksontj commented May 15, 2019

jacksontj commented May 15, 2019

brian-brazil commented May 15, 2019

jacksontj commented May 15, 2019

brian-brazil commented May 15, 2019

jacksontj commented May 15, 2019

krasi-georgiev commented May 16, 2019 • edited

krasi-georgiev left a comment

Choose a reason for hiding this comment

jacksontj commented May 16, 2019

krasi-georgiev commented May 16, 2019

jacksontj commented May 16, 2019

brian-brazil commented May 16, 2019

krasi-georgiev May 17, 2019

Choose a reason for hiding this comment

krasi-georgiev commented May 17, 2019

jacksontj commented May 23, 2019

beorn7 commented May 23, 2019

jacksontj commented May 23, 2019

beorn7 commented May 23, 2019

jacksontj commented May 23, 2019

beorn7 commented May 24, 2019

beorn7 commented May 24, 2019

jacksontj commented May 24, 2019 via email

beorn7 commented May 24, 2019

krasi-georgiev commented May 24, 2019

jacksontj commented May 25, 2019

beorn7 commented May 25, 2019

beorn7 commented May 25, 2019

jacksontj commented May 25, 2019

jacksontj commented May 25, 2019

beorn7 commented May 25, 2019

jacksontj commented May 25, 2019

beorn7 commented May 25, 2019

krasi-georgiev commented May 26, 2019

jacksontj commented May 8, 2019 •

edited

krasi-georgiev commented May 16, 2019 •

edited