existing cache values are never used

[kpm]

The cache struct in parser.go seems to be there so that existing values can be reused for performance, however when we look at the getValue method as below, getValue always grabs a new value past the existing length of the c.vs slice. The only other accesses to the cache are to reset it. So this means that values saved on the c.vs slice are never accessed (except for the last one that was just added for a short time). Am I missing something? Thanks.

func (c *cache) getValue() *Value {
	if cap(c.vs) > len(c.vs) {
		c.vs = c.vs[:len(c.vs)+1]
	} else {
		c.vs = append(c.vs, Value{})
	}
	// Do not reset the value, since the caller must properly init it.
	return &c.vs[len(c.vs)-1]
}

Because c.getValue is called every time we parse a json entity, the whole parse tree has another copy in the cache, and this causes excessive memory usage for no apparent benefit. I was able to parse a large file with nested arrays after changing getValue to the below. It was a 1.4GB file, and was killed on my 32GB RAM machine after reaching 26GB resident RAM. When I tried changing getValue to the below, the file was able to be parsed and used about 9.7GB of resident ram.

func (c *cache) getValue() *Value {
	var v Value
	return &v
}

The large file can be downloaded from https://data.acgov.org/datasets/2b026350b5dd40b18ed7a321fdcdba81_0/about , and selecting geojson format.

[kpm]

Ok, I get it now, the cache allows users to avoid memory allocations when pooling. When I benchmarked my change it shows slower performance, and more memory allocation (except for some larger inputs), because the benchmarks all use pooling. A fair comparison would be to compare pooled caching with unpooled and no cache. Benchmarks are attached, but I haven't had time to try disabling pooling.

benchstats_nocache.txt

Some intermediate values that would normally be garbage collected must be saved on the cache, because the cached scenario uses a lot more memory in some cases, possibly for nested arrays? It would be interesting to dig into this further, but I need to do actual work!

One suggestion I might make is to add a setting for a limit on the size of the cache. That way unexpected OOMs could be avoided in long-running servers where the occasional payload is very large.