Use of json.Decoder

[miekg]

We (also) seeing increased memory use (and still some leaks) in our thanos-store (v 0.4).

json.Decoder jumps out:

Showing top 10 nodes out of 18
      flat  flat%   sum%        cum   cum%
  700.75MB 30.94% 30.94%  2263.38MB 99.93%  github.com/improbable-eng/thanos/pkg/block.ReadIndexCache
  453.84MB 20.04% 50.98%   453.84MB 20.04%  encoding/json.(*Decoder).refill
  439.02MB 19.38% 70.36%   439.02MB 19.38%  encoding/json.(*decodeState).literalStore
  355.28MB 15.69% 86.05%   355.28MB 15.69%  reflect.mapassign
  225.06MB  9.94% 95.99%   225.06MB  9.94%  reflect.unsafe_NewArray
   71.43MB  3.15% 99.14%    71.43MB  3.15%  github.com/improbable-eng/thanos/pkg/block.ReadIndexCache.func1
       8MB  0.35% 99.49%  1037.36MB 45.80%  encoding/json.(*decodeState).object
    0.50MB 0.022% 99.51%   225.56MB  9.96%  reflect.MakeSlice
         0     0% 99.51%  1491.21MB 65.84%  encoding/json.(*Decoder).Decode
         0     0% 99.51%   453.84MB 20.04%  encoding/json.(*Decoder).readValue 

Reading this was illuminating: https://ahmet.im/blog/golang-json-decoder-pitfalls/ (although targets older version of Go [1.7]).

We're planning to rip out json.Decoder and move everything to json.Unmarshal and see how that fares.

[bwplotka]

Yea, so there is overall quite large of improvements we can make to Thanos Store #448

I fully agree here with Decoder not being designed for this 👍 But I think all those 3 flaws are either fixed (not draining connections) in Go1.12 or not affecting us really (ignoring errs, "it's not for non stream"), am I wrong?

[brancz]

If we do consider optimizing json decoding we should probably consider json-iterator. And of course benchmark to validate that it would indeed be better for our use.

[GiedriusS]

I have been working on this for a while: #1013. Should help with the JSON encoding/decoding part.

And I have deliberated a bit on the problems in this issue that you mentioned @miekg as well, and AFAICT @bwplotka is correct in saying that this doesn't affect us since we control the format of the JSON files, and all of the other problems were fixed already.

[miekg]

[ Quoting <[email protected]> in "Re: [improbable-eng/thanos] Use of ..." ]

I have been working on this for a while: #1013. And I have deliberated a bit on the problems in this issue as well, and AFAICT @bwplotka is correct in saying that this doesn't affect us since we control the format of the JSON files, and all of the other problems were fixed already.

Oh yes, that looks nicer still. How far off are you with this PR?

[miekg]

Ok, we've been running a couple of instances with json.Unmarshal instead of Decoder and from eyeballing the memory graphs they look marginally better than the unpatched instance.

It doesn't look it's worth pursuing this, esp in light of #1013 and other efforts to look a memory.

[GiedriusS]

I think we can still do this because the json decoding code will still be in Thanos for some time since the migration path will have to be there. So you can go ahead and do this if it really helps with RAM consumpion IMO, @miekg.

[miekg]

Ack. We'll upstream what we have @tmhdgsn

[miekg]

So even without #1173 but with 1.12.5 the memory profile looks a lot better. The RRS actually drops down and stopped showing this upwards trend that you saw with older Go version.

Once #1173 is in, we'll update again and just run from master probably.

[GiedriusS]

Closing this since #1173 was merged. Thank you! 👍