Memory improvements in FastJsonNode #5088

ashish-goswami · 2020-04-03T07:58:31Z

Dear contributor, what does this PR do?

Fixes #5124, DGRAPH-1170

Motivation

While converting a subgraph to JSON response, an intermediate data structure called
fastJsonNode tree is formed. We have observed when response to be returned is big(specially in recurse queries), this datastructure itself can occupy lot of memory and leading to OOM in some
cases.
This PR aims to reduce space occupied by fastJsonNode tree. fastJsonNode tree is kind of n-ary
tree, where each fastJsonNode maintains some meta data and list of its children. This PR tries to
reduce space occupied by each node in following way:

For each response a separate datastructure called encoder is formed which is responsible for
maintaining meta data for all fastJsonNodes.
encoder has metaSlice and childrenMap where all meta and children list are maintained for
all fastJsonNodes. Index at which meta for a fastJsonNode is present, becomes its value and hence type of a fastJsonNode is uint32.
meta for a fastJsonNode(present at int(fastJsonNode) value in metaSlice) is of uint64 type. It stores all the info for a fastJsonNode. Most significant bit stores value of List field, bytes 7-6 stores attr id and bytes 4 to 1 stores arena offset((explained below)).
encoder has attrMap which has mapping of predicates to unique uint16 number.
encoder also has arena. arena is a larger []byte, which stores bytes for each leaf node. It
offsets are stored in fastJsonNode meta. arena stores same []byte only once and keeps a map
for memhash([]byte) to offset mapping.

On this change, I am able to run some of queries which were resulting in OOM current master.

Components affected by this PR

Mostly query encoding/response.

Does this PR break backwards compatibility?

No

Fixes

More

Profile for a query when RSS usage was around 30GB
master profile on the query:

File: dgraph
Build ID: 4009644c7dfb41957358d88f228b977b2fb552c7
Type: inuse_space
Time: Apr 11, 2020 at 10:31pm (IST)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) top
Showing nodes accounting for 26.74GB, 98.87% of 27.05GB total
Dropped 133 nodes (cum <= 0.14GB)
Showing top 10 nodes out of 42
      flat  flat%   sum%        cum   cum%
   16.62GB 61.43% 61.43%    16.62GB 61.43%  github.com/dgraph-io/dgraph/query.makeScalarNode
    4.23GB 15.63% 77.06%     4.23GB 15.63%  github.com/dgraph-io/dgraph/query.(*fastJsonNode).New
    2.03GB  7.51% 84.57%     2.03GB  7.51%  github.com/dgraph-io/dgraph/query.stringJsonMarshal
    1.64GB  6.08% 90.65%    15.99GB 59.10%  github.com/dgraph-io/dgraph/query.(*fastJsonNode).AddListValue
    0.88GB  3.25% 93.90%     5.24GB 19.36%  github.com/dgraph-io/dgraph/query.(*fastJsonNode).SetUID
    0.44GB  1.61% 95.52%     0.44GB  1.61%  github.com/dgraph-io/dgraph/query.(*fastJsonNode).AddListChild
    0.36GB  1.32% 96.84%     0.39GB  1.45%  github.com/dgraph-io/dgraph/worker.(*queryState).handleValuePostings.func1
    0.25GB  0.92% 97.76%     0.25GB  0.92%  github.com/dgraph-io/ristretto.newCmRow
    0.16GB   0.6% 98.36%     0.16GB   0.6%  github.com/dgraph-io/badger/v2/skl.newArena
    0.14GB  0.51% 98.87%     0.14GB  0.51%  github.com/dgraph-io/ristretto/z.(*Bloom).Size

This PR profile:

File: dgraph
Build ID: 8fd737a95d4edf3ffb305638081766fd0044e99d
Type: inuse_space
Time: Apr 15, 2020 at 11:20am (IST)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) top
Showing nodes accounting for 15557.02MB, 98.53% of 15789.61MB total
Dropped 168 nodes (cum <= 78.95MB)
Showing top 10 nodes out of 60
      flat  flat%   sum%        cum   cum%
 6341.11MB 40.16% 40.16%  6341.11MB 40.16%  github.com/dgraph-io/dgraph/query.(*encoder).appendAttrs
 4598.84MB 29.13% 69.29%  4598.84MB 29.13%  bytes.makeSlice
 3591.16MB 22.74% 92.03%  3591.16MB 22.74%  github.com/dgraph-io/dgraph/query.(*encoder).newNode
  365.52MB  2.31% 94.34%   408.54MB  2.59%  github.com/dgraph-io/dgraph/worker.(*queryState).handleValuePostings.func1
     256MB  1.62% 95.97%      256MB  1.62%  github.com/dgraph-io/ristretto.newCmRow
  166.41MB  1.05% 97.02%   166.41MB  1.05%  github.com/dgraph-io/badger/v2/skl.newArena
  140.25MB  0.89% 97.91%   140.25MB  0.89%  github.com/dgraph-io/ristretto/z.(*Bloom).Size
   91.12MB  0.58% 98.48%    98.05MB  0.62%  github.com/dgraph-io/dgraph/posting.(*List).Uids
       6MB 0.038% 98.52%   122.23MB  0.77%  github.com/dgraph-io/dgraph/worker.(*queryState).handleUidPostings.func1
    0.64MB 0.004% 98.53%   387.17MB  2.45%  github.com/dgraph-io/ristretto.NewCache

Benchmarks for creating 2M empty fastJsonNodes:

Ran following benchmark on master:

func BenchmarkFastJsonNodeMemory(b *testing.B) {
	for i := 0; i < b.N; i++ {
		var fj *fastJsonNode
		for i := 0; i < 2e6; i++ {
			fj = &fastJsonNode{}
		}
		_ = fj
	}
}

Results:

➜  query git:(master) ✗ go test -v -run ^$ -bench BenchmarkFastJsonNodeMemory -benchmem
[Decoder]: Using assembly version of decoder
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/dgraph/query
BenchmarkFastJsonNodeMemory
BenchmarkFastJsonNodeMemory-8   	       9	 123266803 ns/op	192000500 B/op	 2000007 allocs/op
PASS

Ran following benchmark on this PR:

func BenchmarkFastJsonNodeMemory(b *testing.B) {
	for i := 0; i < b.N; i++ {
		enc := newEncoder()
		var fj fastJsonNode
		for i := 0; i < 2e6; i++ {
			fj = enc.newFastJsonNode()
		}
		_ = fj
	}
}

Resutls:

➜  query git:(ashish/globalmap) ✗ go test -v -run ^$ -bench BenchmarkFastJsonNodeMemory -benchmem
[Decoder]: Using assembly version of decoder
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/dgraph/query
BenchmarkFastJsonNodeMemory
BenchmarkFastJsonNodeMemory-8   	      51	  23226043 ns/op	88541677 B/op	      49 allocs/op
PASS
ok  	github.com/dgraph-io/dgraph/query	1.979s

This change is

Docs Preview:

query/arena.go

query/outputnode.go

manishrjain

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @ashish-goswami)

query/arena.go, line 33 at r2 (raw file):

	x.AssertTrue(size > 0)
	return &arena{
		n:   1,

Don't need this.

query/arena.go, line 34 at r2 (raw file):

	return &arena{
		n:   1,
		buf: make([]byte, 0, size),

Append a byte to the buf, to have the offset start from 1.

query/arena.go, line 38 at r2 (raw file):

}

func (a *arena) put(b []byte) int {

Deal with concurrency issues. Return uint32

query/arena.go, line 51 at r2 (raw file):

}

func (a *arena) get(offset int) []byte {

Deal with concurrency. Take uint32

query/outputnode.go, line 90 at r2 (raw file):

)

func (e *encoder) idForAttr(attr string) uint16 {

Thread safe.

query/outputnode.go, line 102 at r2 (raw file):

}

func (e *encoder) attrForID(id uint16) string {

Thread safe.

query/outputnode.go, line 108 at r2 (raw file):

	// Panic for now.
	panic("id not found in map")

Return empty string. Don't panic.

query/outputnode.go, line 118 at r2 (raw file):

	// Bytes 3-4 contains attr.
	// Bit MSB and (MSB-1) contains isChild and list fields values.
	meta uint64

Move this up.

query/outputnode.go, line 133 at r2 (raw file):

func (fj *fastJsonNode) setIsChild(isChild bool) {
	if isChild {
		fj.meta |= (uint64(1) << 63)

Define consts.

query/outputnode.go, line 139 at r2 (raw file):

func (fj *fastJsonNode) setList(list bool) {
	if list {
		fj.meta |= (uint64(1) << 62)

Define consts.

query/outputnode.go, line 144 at r2 (raw file):

func (fj *fastJsonNode) getAttr() uint16 {
	return uint16((fj.meta & (uint64(0xFFFF) << 32)) >> 32)

Define consts.

query/outputnode.go, line 162 at r2 (raw file):

func (fj *fastJsonNode) getList() bool {
	if (fj.meta & (uint64(1) << 62)) > 0 {

return this.

manishrjain

Btw, use variable encoding instead of fixed 4 bytes for length.

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @ashish-goswami)

* Move most of information in Encoder * Comment outputnode_test.go for now

golangcibot · 2020-04-07T14:50:57Z

query/outputnode.go

+// enc = newEncoder()
+)
+
+func (e *encoder) idForAttr(attr string) uint16 {


receiver name e should be consistent with previous receiver name enc for encoder (from golint)

golangcibot · 2020-04-07T14:50:57Z

query/outputnode.go

+	return e.seqNo
+}
+
+func (e *encoder) attrForID(id uint16) string {


receiver name e should be consistent with previous receiver name enc for encoder (from golint)

golangcibot · 2020-04-07T14:50:58Z

query/outputnode.go

-	fj.attrs = append(fj.attrs, makeScalarNode(attr, false, []byte(fmt.Sprintf("\"%#x\"", uid)),
-		false))
+
+	fj.appendAttrs(enc, []fastJsonNode{enc.makeScalarNode(attr, false, []byte(fmt.Sprintf("\"%#x\"", uid)),


line is 104 characters (from lll)

golangcibot · 2020-04-07T14:50:58Z

query/outputnode.go

-	if cmp == 0 {
-		return n[i].order < n[j].order
-	}
+	cmp := strings.Compare(n.enc.attrForID(n.nodes[i].getAttr(n.enc)), n.enc.attrForID(n.nodes[j].getAttr(n.enc)))


line is 111 characters (from lll)

golangcibot · 2020-04-07T14:50:58Z

query/outputnode.go

+	seqNo   uint16
+	arena   *arena
+
+	root          fastJsonNode


field root is unused (from unused)

golangcibot · 2020-04-07T14:50:59Z

query/outputnode.go

+}
+
+// For debugging.
+func (fj fastJsonNode) printMeta(enc *encoder) {


func fastJsonNode.printMeta is unused (from unused)

* Convert method of fastJsonNode to that of encoder

query/outputnode.go

golangcibot · 2020-04-08T13:44:09Z

query/outputnode.go

+type fastJsonNode int
+
+// newFastJsonNode returns a fastJsonNode with its meta set to 0.
+func (enc *encoder) newFastJsonNode() fastJsonNode {


receiver name enc should be consistent with previous receiver name e for encoder (from golint)

golangcibot · 2020-04-08T13:44:09Z

query/outputnode.go

-func (fj *fastJsonNode) AddListValue(attr string, v types.Val, list bool) {
+// newFastJsonNodeWithAttr returns a fastJsonNode with its attr set to attr,
+// with all other meta set to their default value.
+func (enc *encoder) newFastJsonNodeWithAttr(attr uint16) fastJsonNode {


receiver name enc should be consistent with previous receiver name e for encoder (from golint)

golangcibot · 2020-04-08T13:44:09Z

query/outputnode.go

-	fj.attrs = append(fj.attrs, makeScalarNode(attr, false, []byte(fmt.Sprintf("\"%#x\"", uid)),
-		false))
+
+	enc.appendAttrs(fj, []fastJsonNode{enc.makeScalarNode(attr, false, []byte(fmt.Sprintf("\"%#x\"", uid)),


line is 104 characters (from lll)

manishrjain

This is looking very good. Let's run this again and see what we can optimize.

Reviewed 1 of 3 files at r3, 2 of 2 files at r5.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @ashish-goswami)

query/outputnode.go, line 720 at r5 (raw file):

		return
	}
	g := enc.newFastJsonNodeWithAttr(enc.idForAttr(fname))

Too long. Can be: enc.newNodeWithAttr(fname)?

manishrjain · 2020-04-09T01:19:52Z

The benchmarks are not correct. The first case, is just creating fastJsonNode and not storing it anywhere. The second case is storing them in a slice. So, the first case is under counting the memory usage. Therefore, there's a bigger win here.

In any case, how do the results look with the query and the data?

ashish-goswami

Dismissed @golangcibot from 14 discussions.
Reviewable status: 0 of 3 files reviewed, 15 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @manishrjain)

query/arena.go, line 71 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

Done.

query/arena.go, line 33 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't need this.

Done.

query/arena.go, line 34 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Append a byte to the buf, to have the offset start from 1.

Done.

query/arena.go, line 38 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Deal with concurrency issues. Return uint32

Returning uint32 now.

query/arena.go, line 51 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Deal with concurrency. Take uint32

Taking uint32 now.

query/outputnode.go, line 154 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

S1008: should use 'return ' instead of 'if { return }; return ' (from gosimple)

Done.

query/outputnode.go, line 162 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

S1008: should use 'return ' instead of 'if { return }; return ' (from gosimple)

Done.

query/outputnode.go, line 118 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Move this up.

Done.

query/outputnode.go, line 133 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Define consts.

Done.

query/outputnode.go, line 139 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Define consts.

Done.

query/outputnode.go, line 144 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Define consts.

Done.

query/outputnode.go, line 162 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

return this.

Done.

ashish-goswami

Reviewable status: 0 of 3 files reviewed, 15 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @manishrjain)

query/outputnode.go, line 720 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Too long. Can be: enc.newNodeWithAttr(fname)?

Done.

manishrjain

It's starting to look good. Let's address the comments and then move it to a formal PR and get it in.

Reviewed 3 of 3 files at r6.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @manishrjain)

query/arena.go, line 50 at r6 (raw file):

// Note: for now this function can only put buffers such that:
// varint(len(b)) + len(b) < math.MaxUint32.
func (a *arena) put(b []byte) (uint8, uint32) {

uint64, but just say that the MSB 24 bits are not used.

Also, return error. That'd be preferred. Otherwise, you can return a 0, and then ensure that it gets handled at the place which has error handling.

query/arena.go, line 60 at r6 (raw file):

	if int64(offset+w+len(b)) > int64(math.MaxUint32) {
		buf := make([]byte, 0, minSize)

These bufs can be retrieved from a sync.Pool.

query/arena.go, line 65 at r6 (raw file):

		last = len(a.bufs) - 1
		offset = 1
		x.AssertTruef(last < int(math.MaxUint8),

Do not assert. Let's return error instead.

query/arena.go, line 67 at r6 (raw file):

		x.AssertTruef(last < int(math.MaxUint8),
			"Number of bufs in arena should be < math.MaxUint8")
		x.AssertTruef(int64(offset+w+len(b)) < int64(math.MaxUint32),

Asserting won't help, a user can then take down Dgraph server. Return error.

query/arena.go, line 77 at r6 (raw file):

}

func (a *arena) get(idx uint8, offset uint32) []byte {

uint64. Also, let's return error.

query/arena.go, line 84 at r6 (raw file):

	// First read length, then read actual buffer.
	size, r := binary.Varint(a.bufs[idx][offset:])

I'd check whether the bufs idx is valid and also if the offset is valid. And return error.

query/outputnode.go, line 133 at r6 (raw file):

const (
	msbBit       = 0x8000000000000000

Add comments for each and every one of them.

query/outputnode.go, line 136 at r6 (raw file):

	secondMsbBit = 0x4000000000000000
	setBytes76   = 0x00FFFF0000000000
	unsetBytes76 = 0xFF0000FFFFFFFFFF

unsetBytes76 = ^setBytes76 // however Go does bit flipping.

golangcibot · 2020-04-14T09:55:28Z

query/arena.go

+)
+
+const (
+	minSize = 1 * 1024 // 1 KB


minSize is unused (from varcheck)

* Also address some of review comments

golangcibot · 2020-04-14T15:33:35Z

query/outputnode.go

 					if sg.Params.GetUid {
-						uc.SetUID(childUID, "uid")
+						enc.SetUID(uc, childUID, enc.idForAttr("uid"))


Error return value of enc.SetUID is not checked (from errcheck)

golangcibot · 2020-04-14T15:33:36Z

query/outputnode.go

 		default:
 			if pc.Params.Alias == "" && len(pc.Params.Langs) > 0 && pc.Params.Langs[0] != "*" {
 				fieldName += "@"
 				fieldName += strings.Join(pc.Params.Langs, ":")
 			}

 			if pc.Attr == "uid" {
-				dst.SetUID(uid, pc.fieldName())
+				enc.SetUID(dst, uid, enc.idForAttr(pc.fieldName()))


Error return value of enc.SetUID is not checked (from errcheck)

golangcibot · 2020-04-14T15:33:36Z

query/outputnode.go

-	if (sg.Params.GetUid && !dst.IsEmpty()) || sg.Params.Shortest {
-		dst.SetUID(uid, "uid")
+	if (sg.Params.GetUid && !enc.IsEmpty(dst)) || sg.Params.Shortest {
+		enc.SetUID(dst, uid, enc.idForAttr("uid"))


Error return value of enc.SetUID is not checked (from errcheck)

golangcibot · 2020-04-14T15:33:36Z

query/arena.go

+	errArenaFull     = errors.New("arena is full")
+	errInvalidOffset = errors.New("arena get performed with invalid offset")
+
+	maxArenaSize = math.MaxUint32


maxArenaSize is unused (from varcheck)

ashish-goswami

Reviewable status: 0 of 4 files reviewed, 28 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @manishrjain)

query/arena.go, line 60 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

These bufs can be retrieved from a sync.Pool.

Done.

query/arena.go, line 65 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Do not assert. Let's return error instead.

Removed this for now, only using single buffer.

query/arena.go, line 77 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

uint64. Also, let's return error.

Done.

query/arena.go, line 84 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'd check whether the bufs idx is valid and also if the offset is valid. And return error.

Done.

query/arena.go, line 28 at r7 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

minSize is unused (from varcheck)

Removed it.

query/arena.go, line 32 at r8 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

maxArenaSize is unused (from varcheck)

Getting used now.

query/outputnode.go, line 108 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Return empty string. Don't panic.

Done.

query/outputnode.go, line 133 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add comments for each and every one of them.

Done.

query/outputnode.go, line 1191 at r8 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of enc.SetUID is not checked (from errcheck)

Done.

query/outputnode.go, line 1261 at r8 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of enc.SetUID is not checked (from errcheck)

Done.

manishrjain

Reviewed 2 of 4 files at r8, 2 of 2 files at r9.
Reviewable status: all files reviewed, 31 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @manishrjain)

query/arena.go, line 50 at r9 (raw file):

	// Start offset from 1, to avoid reading bytes when offset is storing default value(0) in
	// fastJsonNode. Hence append dummy byte.
	buf := make([]byte, 0, size)

This needs to be in a pool.

query/arena.go, line 72 at r9 (raw file):

	}
	// First put length of buffer(varint encoded), then put actual buffer.
	sizeBuf := (a.sizeBufPool.Get()).([]byte)

No, don't use pool for this.

query/outputnode.go, line 1230 at r9 (raw file):

					}
					encodeAsList := pc.List && len(lang) == 0
					err := enc.AddListValue(dst, enc.idForAttr(fieldNameWithTag), sv, encodeAsList)

if err := ...; err != nil

gja · 2020-04-14T16:03:19Z

Would you mind writing some unit tests for this. I've started with a few simple tests in fastjson_test.go

ashish-goswami · 2020-04-16T15:04:26Z

Would you mind writing some unit tests for this. I've started with a few simple tests in fastjson_test.go

I have added some tests for fastJsonNode.

manishrjain

Great work!

Reviewed 3 of 3 files at r10.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @manishrjain, and @pawanrawal)

query/arena.go, line 72 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No, don't use pool for this.

var sizeBuf [binary.MaxVarintLen64]byte

query/arena.go, line 76 at r10 (raw file):

	w := binary.PutVarint(sizeBuf, int64(len(b)))
	offset := len(a.buf)
	if int64(len(a.buf)+w+len(b)) > int64(maxArenaSize) {

maxArenaSize can just be int64.

query/arena.go, line 77 at r10 (raw file):

	offset := len(a.buf)
	if int64(len(a.buf)+w+len(b)) > int64(maxArenaSize) {
		return 0, errArenaFull

Do mention the size of the arena.

query/outputnode.go, line 136 at r6 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

unsetBytes76 = ^setBytes76 // however Go does bit flipping.

unsetBytes76 = uint64(^setBytes76)

query/outputnode.go, line 113 at r10 (raw file):

	// TODO(Ashish): check for overflow.
	enc.seqNo++

just return error if possible.

query/outputnode.go, line 148 at r10 (raw file):

	msbBit = 0x8000000000000000
	// Value with all bits set to 1 for bytes 7 and 6.
	setBytes76 = 0x00FFFF0000000000

setBytes76 uint64 = ...

query/outputnode.go, line 786 at r10 (raw file):

}

func handleCountUIDNodes(sg *SubGraph, enc *encoder, n fastJsonNode, count int) (bool, error) {

Either make it part of subgraph, or encoder.

Fixes #5124, DGRAPH-1170 While converting a subgraph to JSON response, an intermediate data structure called fastJsonNode tree is formed. We have observed when response to be returned is big(specially in recurse queries), this datastructure itself can occupy lot of memory and leading to OOM in some cases. This PR aims to reduce space occupied by fastJsonNode tree. fastJsonNode tree is kind of n-ary tree, where each fastJsonNode maintains some meta data and list of its children. This PR tries to reduce space occupied by each node in following way: For each response a separate datastructure called encoder is formed which is responsible for maintaining meta data for all fastJsonNodes. encoder has metaSlice and childrenMap where all meta and children list are maintained for all fastJsonNodes. Index at which meta for a fastJsonNode is present, becomes its value and hence type of a fastJsonNode is uint32. meta for a fastJsonNode(present at int(fastJsonNode) value in metaSlice) is of uint64 type. It stores all the info for a fastJsonNode. Most significant bit stores value of List field, bytes 7-6 stores attr id and bytes 4 to 1 stores arena offset((explained below)). encoder has attrMap which has mapping of predicates to unique uint16 number. encoder also has arena. arena is a larger []byte, which stores bytes for each leaf node. It offsets are stored in fastJsonNode meta. arena stores same []byte only once and keeps a map for memhash([]byte) to offset mapping. On this change, I am able to run some of queries which were resulting in OOM current master. Profile for a query when RSS usage was around 30GB master profile on the query: File: dgraph Build ID: 4009644c7dfb41957358d88f228b977b2fb552c7 Type: inuse_space Time: Apr 11, 2020 at 10:31pm (IST) Entering interactive mode (type "help" for commands, "o" for options) (pprof) top Showing nodes accounting for 26.74GB, 98.87% of 27.05GB total Dropped 133 nodes (cum <= 0.14GB) Showing top 10 nodes out of 42 flat flat% sum% cum cum% 16.62GB 61.43% 61.43% 16.62GB 61.43% github.com/dgraph-io/dgraph/query.makeScalarNode 4.23GB 15.63% 77.06% 4.23GB 15.63% github.com/dgraph-io/dgraph/query.(*fastJsonNode).New 2.03GB 7.51% 84.57% 2.03GB 7.51% github.com/dgraph-io/dgraph/query.stringJsonMarshal 1.64GB 6.08% 90.65% 15.99GB 59.10% github.com/dgraph-io/dgraph/query.(*fastJsonNode).AddListValue 0.88GB 3.25% 93.90% 5.24GB 19.36% github.com/dgraph-io/dgraph/query.(*fastJsonNode).SetUID 0.44GB 1.61% 95.52% 0.44GB 1.61% github.com/dgraph-io/dgraph/query.(*fastJsonNode).AddListChild 0.36GB 1.32% 96.84% 0.39GB 1.45% github.com/dgraph-io/dgraph/worker.(*queryState).handleValuePostings.func1 0.25GB 0.92% 97.76% 0.25GB 0.92% github.com/dgraph-io/ristretto.newCmRow 0.16GB 0.6% 98.36% 0.16GB 0.6% github.com/dgraph-io/badger/v2/skl.newArena 0.14GB 0.51% 98.87% 0.14GB 0.51% github.com/dgraph-io/ristretto/z.(*Bloom).Size This PR profile: File: dgraph Build ID: 8fd737a95d4edf3ffb305638081766fd0044e99d Type: inuse_space Time: Apr 15, 2020 at 11:20am (IST) Entering interactive mode (type "help" for commands, "o" for options) (pprof) top Showing nodes accounting for 15557.02MB, 98.53% of 15789.61MB total Dropped 168 nodes (cum <= 78.95MB) Showing top 10 nodes out of 60 flat flat% sum% cum cum% 6341.11MB 40.16% 40.16% 6341.11MB 40.16% github.com/dgraph-io/dgraph/query.(*encoder).appendAttrs 4598.84MB 29.13% 69.29% 4598.84MB 29.13% bytes.makeSlice 3591.16MB 22.74% 92.03% 3591.16MB 22.74% github.com/dgraph-io/dgraph/query.(*encoder).newNode 365.52MB 2.31% 94.34% 408.54MB 2.59% github.com/dgraph-io/dgraph/worker.(*queryState).handleValuePostings.func1 256MB 1.62% 95.97% 256MB 1.62% github.com/dgraph-io/ristretto.newCmRow 166.41MB 1.05% 97.02% 166.41MB 1.05% github.com/dgraph-io/badger/v2/skl.newArena 140.25MB 0.89% 97.91% 140.25MB 0.89% github.com/dgraph-io/ristretto/z.(*Bloom).Size 91.12MB 0.58% 98.48% 98.05MB 0.62% github.com/dgraph-io/dgraph/posting.(*List).Uids 6MB 0.038% 98.52% 122.23MB 0.77% github.com/dgraph-io/dgraph/worker.(*queryState).handleUidPostings.func1 0.64MB 0.004% 98.53% 387.17MB 2.45% github.com/dgraph-io/ristretto.NewCache

Fixes dgraph-io#5124, DGRAPH-1170 While converting a subgraph to JSON response, an intermediate data structure called fastJsonNode tree is formed. We have observed when response to be returned is big(specially in recurse queries), this datastructure itself can occupy lot of memory and leading to OOM in some cases. This PR aims to reduce space occupied by fastJsonNode tree. fastJsonNode tree is kind of n-ary tree, where each fastJsonNode maintains some meta data and list of its children. This PR tries to reduce space occupied by each node in following way: For each response a separate datastructure called encoder is formed which is responsible for maintaining meta data for all fastJsonNodes. encoder has metaSlice and childrenMap where all meta and children list are maintained for all fastJsonNodes. Index at which meta for a fastJsonNode is present, becomes its value and hence type of a fastJsonNode is uint32. meta for a fastJsonNode(present at int(fastJsonNode) value in metaSlice) is of uint64 type. It stores all the info for a fastJsonNode. Most significant bit stores value of List field, bytes 7-6 stores attr id and bytes 4 to 1 stores arena offset((explained below)). encoder has attrMap which has mapping of predicates to unique uint16 number. encoder also has arena. arena is a larger []byte, which stores bytes for each leaf node. It offsets are stored in fastJsonNode meta. arena stores same []byte only once and keeps a map for memhash([]byte) to offset mapping. On this change, I am able to run some of queries which were resulting in OOM current master. Profile for a query when RSS usage was around 30GB master profile on the query: File: dgraph Build ID: 4009644c7dfb41957358d88f228b977b2fb552c7 Type: inuse_space Time: Apr 11, 2020 at 10:31pm (IST) Entering interactive mode (type "help" for commands, "o" for options) (pprof) top Showing nodes accounting for 26.74GB, 98.87% of 27.05GB total Dropped 133 nodes (cum <= 0.14GB) Showing top 10 nodes out of 42 flat flat% sum% cum cum% 16.62GB 61.43% 61.43% 16.62GB 61.43% github.com/dgraph-io/dgraph/query.makeScalarNode 4.23GB 15.63% 77.06% 4.23GB 15.63% github.com/dgraph-io/dgraph/query.(*fastJsonNode).New 2.03GB 7.51% 84.57% 2.03GB 7.51% github.com/dgraph-io/dgraph/query.stringJsonMarshal 1.64GB 6.08% 90.65% 15.99GB 59.10% github.com/dgraph-io/dgraph/query.(*fastJsonNode).AddListValue 0.88GB 3.25% 93.90% 5.24GB 19.36% github.com/dgraph-io/dgraph/query.(*fastJsonNode).SetUID 0.44GB 1.61% 95.52% 0.44GB 1.61% github.com/dgraph-io/dgraph/query.(*fastJsonNode).AddListChild 0.36GB 1.32% 96.84% 0.39GB 1.45% github.com/dgraph-io/dgraph/worker.(*queryState).handleValuePostings.func1 0.25GB 0.92% 97.76% 0.25GB 0.92% github.com/dgraph-io/ristretto.newCmRow 0.16GB 0.6% 98.36% 0.16GB 0.6% github.com/dgraph-io/badger/v2/skl.newArena 0.14GB 0.51% 98.87% 0.14GB 0.51% github.com/dgraph-io/ristretto/z.(*Bloom).Size This PR profile: File: dgraph Build ID: 8fd737a95d4edf3ffb305638081766fd0044e99d Type: inuse_space Time: Apr 15, 2020 at 11:20am (IST) Entering interactive mode (type "help" for commands, "o" for options) (pprof) top Showing nodes accounting for 15557.02MB, 98.53% of 15789.61MB total Dropped 168 nodes (cum <= 78.95MB) Showing top 10 nodes out of 60 flat flat% sum% cum cum% 6341.11MB 40.16% 40.16% 6341.11MB 40.16% github.com/dgraph-io/dgraph/query.(*encoder).appendAttrs 4598.84MB 29.13% 69.29% 4598.84MB 29.13% bytes.makeSlice 3591.16MB 22.74% 92.03% 3591.16MB 22.74% github.com/dgraph-io/dgraph/query.(*encoder).newNode 365.52MB 2.31% 94.34% 408.54MB 2.59% github.com/dgraph-io/dgraph/worker.(*queryState).handleValuePostings.func1 256MB 1.62% 95.97% 256MB 1.62% github.com/dgraph-io/ristretto.newCmRow 166.41MB 1.05% 97.02% 166.41MB 1.05% github.com/dgraph-io/badger/v2/skl.newArena 140.25MB 0.89% 97.91% 140.25MB 0.89% github.com/dgraph-io/ristretto/z.(*Bloom).Size 91.12MB 0.58% 98.48% 98.05MB 0.62% github.com/dgraph-io/dgraph/posting.(*List).Uids 6MB 0.038% 98.52% 122.23MB 0.77% github.com/dgraph-io/dgraph/worker.(*queryState).handleUidPostings.func1 0.64MB 0.004% 98.53% 387.17MB 2.45% github.com/dgraph-io/ristretto.NewCache

ashish-goswami added 4 commits April 1, 2020 18:04

Have map for attrs

3f0174e

Change makeScalarNode to be more clear in profile

3ae6646

Remove order from fastJsonNode, as it is not getting used properly

2b8d9f0

Have composite for all fastJsonNode attributes

18b6e0b

golangcibot reviewed Apr 3, 2020

View reviewed changes

query/arena.go Outdated Show resolved Hide resolved

query/arena.go Show resolved Hide resolved

query/arena.go Show resolved Hide resolved

query/outputnode.go Outdated Show resolved Hide resolved

query/outputnode.go Outdated Show resolved Hide resolved

ashish-goswami added 2 commits April 3, 2020 15:19

Fix minor bug

f0f3034

Avoid copying in arena get

4df1c37

manishrjain reviewed Apr 3, 2020

View reviewed changes

ashish-goswami added 2 commits April 6, 2020 20:02

Address review comments except concurrency

b694762

Have interger based fastJsonNode

84b061b

* Move most of information in Encoder * Comment outputnode_test.go for now

golangcibot reviewed Apr 7, 2020

View reviewed changes

Refactor code

0453a39

* Convert method of fastJsonNode to that of encoder

golangcibot reviewed Apr 8, 2020

View reviewed changes

Minor change

f600f1d

manishrjain reviewed Apr 9, 2020

View reviewed changes

ashish-goswami added 3 commits April 9, 2020 18:22

More refactoring

2c0db78

Add benchmark for memory

a2361da

Add review comments

a1b50b4

ashish-goswami commented Apr 9, 2020

View reviewed changes

ashish-goswami added 2 commits April 9, 2020 21:02

Minor change

8e4d7c2

Support for multiple buffers in arena

b279758

manishrjain reviewed Apr 13, 2020

View reviewed changes

ashish-goswami added 2 commits April 14, 2020 13:37

Change fastJsonNode type to uint32

ade4537

Have map in arena to store buffer only once

fa3d15f

golangcibot reviewed Apr 14, 2020

View reviewed changes

query/arena.go Outdated

)

const (

minSize = 1 * 1024 // 1 KB

Copy link

golangcibot Apr 14, 2020

Choose a reason for hiding this comment

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

minSize is unused (from varcheck)

ashish-goswami added 2 commits April 14, 2020 17:35

Merge remote-tracking branch 'origin/master' into ashish/globalmap

f7e23b5

Remove support for mutliple buffers in arena

9eb709c

ashish-goswami added 2 commits April 14, 2020 20:47

Return error from Arena put if not enough space is left

6afb8d4

* Also address some of review comments

Return error from arena get

70e2a48

golangcibot reviewed Apr 14, 2020

View reviewed changes

Minor changes

aa23b71

ashish-goswami commented Apr 14, 2020

View reviewed changes

manishrjain reviewed Apr 14, 2020

View reviewed changes

ashish-goswami added 2 commits April 15, 2020 18:36

Have pool for arena

b2e7161

Add tests for fastJsonNode

ed45421

ashish-goswami marked this pull request as ready for review April 16, 2020 05:10

ashish-goswami requested review from pawanrawal and a team as code owners April 16, 2020 05:10

ashish-goswami changed the title ~~[WIP] Improvements in FastJsonNode~~ Improvements in FastJsonNode Apr 16, 2020

ashish-goswami added 2 commits April 16, 2020 14:04

Minor changes

77f4d88

Fix deepsource warnings

d50f623

manishrjain approved these changes Apr 16, 2020

View reviewed changes

Add review comments

9ab57b8

ashish-goswami changed the title ~~Improvements in FastJsonNode~~ Memory improvements in FastJsonNode Apr 20, 2020

ashish-goswami merged commit d3a4305 into master Apr 20, 2020

ashish-goswami deleted the ashish/globalmap branch April 20, 2020 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory improvements in FastJsonNode #5088

Memory improvements in FastJsonNode #5088

ashish-goswami commented Apr 3, 2020 •

edited

Loading

manishrjain left a comment

manishrjain left a comment

golangcibot Apr 7, 2020

golangcibot Apr 7, 2020

golangcibot Apr 7, 2020

golangcibot Apr 7, 2020

golangcibot Apr 7, 2020

golangcibot Apr 7, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

golangcibot Apr 8, 2020

manishrjain left a comment

manishrjain commented Apr 9, 2020

ashish-goswami left a comment

ashish-goswami left a comment

manishrjain left a comment

golangcibot Apr 14, 2020

golangcibot Apr 14, 2020

golangcibot Apr 14, 2020

golangcibot Apr 14, 2020

golangcibot Apr 14, 2020

ashish-goswami left a comment

manishrjain left a comment

gja commented Apr 14, 2020

ashish-goswami commented Apr 16, 2020

manishrjain left a comment

Memory improvements in FastJsonNode #5088

Memory improvements in FastJsonNode #5088

Conversation

ashish-goswami commented Apr 3, 2020 • edited Loading

Dear contributor, what does this PR do?

Motivation

Components affected by this PR

Does this PR break backwards compatibility?

Fixes

More

manishrjain left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

manishrjain commented Apr 9, 2020

ashish-goswami left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

gja commented Apr 14, 2020

ashish-goswami commented Apr 16, 2020

manishrjain left a comment

Choose a reason for hiding this comment

ashish-goswami commented Apr 3, 2020 •

edited

Loading