Improve codec performance #916

2opremio · 2016-02-04T15:31:19Z

Improves #854

Fix tests
Generate encoders/decoders on the fly (or at least incorporate a make target for it)
Fix hack from (94cee66) by updating all the vendored k8s dependencies (sigh)
Rebase

2opremio · 2016-02-09T17:58:10Z

As mentioned in #854 (comment), using ffjson without code generation helps a bit but doesn't cut it.

I've tried the ffjson code generator. However, the encoder generated from https://github.com/weaveworks/scope/blob/master/report/node.go breaks the UI, that is:

go get -u github.com/pquerna/ffjson
cd $GOPATH/src/github.com/weaveworks/scope/report
ffjson node.go

Which results in:

I really don't want to debug the generated code, so I am going to try https://github.com/ugorji/go/ next

2opremio · 2016-02-10T19:21:45Z

Using compile-time generated JSON encoders/decoders with github.com/ugorji/go/codec doesn't seem to solve the problem either. Here are the profiles from the scenario described in #854 (comment) #812

I run scope in the service to test (5 probes 1 app). Here's the app's profile:

pprof.localhost:4040.samples.cpu.009.pb.gz

And here's the probe's profile:

pprof.localhost:4041.samples.cpu.003.pb.gz

As much as we would like to use json, I think it's time to move on to a faster codec. We can use part of this work for the communication with the UI (which the profile above is not taking into account).

2opremio · 2016-02-10T20:01:56Z

I think I am going to try gob+websockets next.

2opremio · 2016-02-11T20:19:38Z

The websocket+gob approach didn't help either (see #957 (comment) )

I replaced all the {Marhsal,Unmarhsal}JSON methods by Selfers and it helps a lot with the garbage collection because we don't need to create more intermediate buffers (or encoders/decoders). Note how gzip now takes 30% of the decoding time.

App:

pprof.localhost:4040.samples.cpu.010.pb.gz

Probe:

pprof.localhost:4041.samples.cpu.004.pb.gz

The Selfers now allow to move to faster codecs like msgpack very easily. I am going to try that next.

2opremio · 2016-02-12T11:39:50Z

After moving to msgpack and updating the ugorji/go/codec library I get this

App:

Probe:

Although the profile looks the same for the app, note how the duration has gone down considerably. The probe doesn't seem to improve much (it doesn't degrade either) so let's treat that separately.

Note that this is still measured without connecting the UI (i.e. browser), but it brings down CPU usage of the app in the service (5 probes) to <10% compared to ~70% in master .

The next steps would be:

Reduce the size/publish-frequency of the reports
Tweak the gzip compression level.
Increase GOGC? (sounds like cheating, but may be worth trading some memory for GC CPU)
Look into building github.com/ugorji/go/codec/ with -tags=unsafe to reduce decoding allocations. I tried, but it basically broke everything. I am going to create a ticket upstream.
Create a custom encoder/decoder (I wouldn't be up for this, but @bboreham has a lot of experience and may want to volunteer :) ).

@paulbellamy @peterbourgon Can you please take a look at this and give some feedback? If you are OK with the results/approach, I will go ahead and clean it up for code review.

app/router.go

-			decoder = json.NewDecoder(reader).Decode
+			decoder = func(i interface{}) error {
+				return codec.NewDecoder(reader, &codec.JsonHandle{}).Decode(i)
+			}


paulbellamy · 2016-02-12T13:32:56Z

This seems to have trouble with children's metrics.

2opremio · 2016-02-12T14:12:21Z

This seems to have trouble with children's metrics.

Thanks, I will look into it but I will fix the tests first to see if that catches it.

Here's a final %CPU comparison against 26ccb49 running Scope in the service (5 probes -> 1 app)

What	`26ccb49`	This PR
Probe(s)	15-25%	20%
App (no UI)	70%	10%
App (with UI)	-	30% with peaks of 50%

(I cannot simply compare against master because it's broken on the service due to #955).

For reference, here's the profile of the App with the UI attached:

The codec is not the culprit (i.e. unrelated to this PR).

paulbellamy · 2016-02-12T14:25:54Z

Thanks, I will look into that but will fix the tests first to see if that catches it.

I suspect it is with json encoding of metrics, so it won't...

This caused a dependency chain reaction (sigh): * All the k8s packages had to be fetched again. This in turn required: * Pining github.com/docker/docker/pkg/parsers to 0f5c9d301b9b1cca66b3ea0f9dec3b5317d3686d to cirvumvent kubernetes/kubernetes#18774 * Update github.com/juju/ratelimit * Make probe/kubernetes/client.go comply with API changes.

2opremio · 2016-02-16T06:44:41Z

I suspect it is with json encoding of metrics, so it won't...

@paulbellamy What problems were you seeing with the metrics? I have noticed that the new codec outputs floats instead of ints. e.g:

        "metrics": {
          "docker_memory_usage": {
            "samples": [
              {
                "date": "2016-02-16T05:24:00.72055402Z",
                "value": 36864.0
              },

instead of:

        "metrics": {
          "docker_memory_usage": {
            "samples": [
              {
                "date":  "2016-02-16T05:30:54.719541452Z",
                "value": 36864
              },

paulbellamy · 2016-02-16T10:18:09Z

I was just never seeing any values rendered for them on children. Didn't look further into it than that.

foot · 2016-02-16T14:22:51Z

Sorry i clicked on the checkboxes on the first comment, not really paying attention to what I was doing!

Hope they're in the right state.

2opremio · 2016-02-16T16:40:34Z

OK, so this is why controls are not working: ugorji/go#141 .

The new codec library misbehaves when embedding types, which is exactly how our control details are defined, sigh.

This may also be what's causing problems with the metrics.

I think I may have a workaround though: generate selfers for all the types we serialize.

@foot No worries

paulbellamy · 2016-02-16T17:10:31Z

This may also be what's causing problems with the metrics.

@2opremio yep.

2opremio · 2016-02-16T23:23:18Z

OK, I found yet another bug, ugorji/go#142 which I worked around too.

The controls are working and I think the metrics are alright.

@paulbellamy PTAL, I think I will open a bottle of champaign when this gets merged.

To workaround ugorji/go#141

paulbellamy · 2016-02-17T10:59:45Z

Why are we using your own fork of the codec, vs. the upstream?

paulbellamy · 2016-02-17T11:23:08Z

let's ditch GOHOSTOS and GOHOSTARCH and do unset GOOS GOARCH foo instead, so go can just auto-detect. Less to learn for the user of the makefile.

2opremio · 2016-02-17T11:47:24Z

Why are we using your own fork of the codec, vs. the upstream?

The fork is only used to build codecgen. It's a temporary solution, see the commit comment:

I included
https://github.com/2opremio/go-1/tree/master/codec/codecgen instead of
upstream until ugorji/go#139 is merged.

In fact we will be able to use upstream one once this is fixed: ugorji/go#140 (comment) , which I guess will be pretty soon judging by the maintainer's responsiveness.

EDIT: I added #973

2opremio · 2016-02-17T11:49:58Z

let's ditch GOHOSTOS and GOHOSTARCH and do unset GOOS GOARCH foo instead, so go can just auto-detect

Agreed

paulbellamy · 2016-02-17T12:36:31Z

the secondary expansion thing is terribly complex and obtuse.

2opremio · 2016-02-17T13:19:43Z

@paulbellamy please take another look

paulbellamy · 2016-02-17T13:47:17Z

Much simpler, thanks. :) LGTM

Improve codec performance

2opremio mentioned this pull request Feb 4, 2016

App using lots of CPU #854

Closed

2opremio force-pushed the 854-reports-codec branch 3 times, most recently from acc8383 to b46d5d1 Compare February 9, 2016 16:30

2opremio force-pushed the 854-reports-codec branch 3 times, most recently from c9f1a2b to f40cc7c Compare February 10, 2016 15:56

2opremio mentioned this pull request Feb 11, 2016

[WIP] Transmit reports using gob with websockets ... #957

Closed

paulbellamy reviewed Feb 12, 2016
View reviewed changes

app/router.go

decoder = json.NewDecoder(reader).Decode

decoder = func(i interface{}) error {

return codec.NewDecoder(reader, &codec.JsonHandle{}).Decode(i)

}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

2opremio force-pushed the 854-reports-codec branch 11 times, most recently from 84bf578 to 9b781a7 Compare February 16, 2016 06:04

2opremio force-pushed the 854-reports-codec branch from 9b781a7 to 9f02e20 Compare February 16, 2016 06:33

2opremio changed the title ~~[WIP] Improve report codec performance~~ Improve report codec performance Feb 16, 2016

2opremio changed the title ~~Improve report codec performance~~ Improve codec performance Feb 16, 2016

2opremio force-pushed the 854-reports-codec branch 3 times, most recently from 7c3c2f6 to 0ff1d16 Compare February 16, 2016 23:48

Alfonso Acosta added 2 commits February 16, 2016 23:52

Implement Selfers for all render types

9c368c4

To workaround ugorji/go#141

Circumvent ugorji/go#142

bdc97d6

2opremio force-pushed the 854-reports-codec branch from 0ff1d16 to bdc97d6 Compare February 16, 2016 23:52

Review comments

359919b

2opremio pushed a commit that referenced this pull request Feb 17, 2016

Merge pull request #916 from weaveworks/854-reports-codec

a2710ec

Improve codec performance

2opremio merged commit a2710ec into master Feb 17, 2016

2opremio deleted the 854-reports-codec branch February 17, 2016 14:07

This was referenced Feb 17, 2016

Remove gob encoding support #971

Open

Compile ugorji/go/codec with -tags unsafe for higher performance #972

Closed

Remove dependency on github.com/2opremio/go-1/codec #973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve codec performance #916

Improve codec performance #916

2opremio commented Feb 4, 2016

2opremio commented Feb 9, 2016

2opremio commented Feb 10, 2016

2opremio commented Feb 10, 2016

2opremio commented Feb 11, 2016

2opremio commented Feb 12, 2016

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Feb 12, 2016

2opremio commented Feb 12, 2016

paulbellamy commented Feb 12, 2016

2opremio commented Feb 16, 2016

paulbellamy commented Feb 16, 2016

foot commented Feb 16, 2016

2opremio commented Feb 16, 2016

paulbellamy commented Feb 16, 2016

2opremio commented Feb 16, 2016

paulbellamy commented Feb 17, 2016

paulbellamy commented Feb 17, 2016

2opremio commented Feb 17, 2016

2opremio commented Feb 17, 2016

paulbellamy commented Feb 17, 2016

2opremio commented Feb 17, 2016

paulbellamy commented Feb 17, 2016

Improve codec performance #916

Improve codec performance #916

Conversation

2opremio commented Feb 4, 2016

2opremio commented Feb 9, 2016

2opremio commented Feb 10, 2016

2opremio commented Feb 10, 2016

2opremio commented Feb 11, 2016

2opremio commented Feb 12, 2016

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Feb 12, 2016

2opremio commented Feb 12, 2016

paulbellamy commented Feb 12, 2016

2opremio commented Feb 16, 2016

paulbellamy commented Feb 16, 2016

foot commented Feb 16, 2016

2opremio commented Feb 16, 2016

paulbellamy commented Feb 16, 2016

2opremio commented Feb 16, 2016

paulbellamy commented Feb 17, 2016

paulbellamy commented Feb 17, 2016

2opremio commented Feb 17, 2016

2opremio commented Feb 17, 2016

paulbellamy commented Feb 17, 2016

2opremio commented Feb 17, 2016

paulbellamy commented Feb 17, 2016