`TreeInterpreter` creates reference cycle, causing GC pressure

[mrry]

We recently noticed that a heavy JMESpath workload was triggering a large number of garbage collection runs. We are using jmespath.compile(), and we tracked this down to the jmespath.visitor.TreeInterpreter that is created on every call to `ParsedResult.search():

jmespath.py/jmespath/parser.py

Line 508 in bbe7300

interpreter = visitor.TreeInterpreter(options)

It appears that TreeInterpreter creates a reference cycle, which leads to the GC being triggered frequently to clean up the cycles. As far as I can tell, the problem comes from the Visitor._method_cache:

jmespath.py/jmespath/visitor.py

Lines 91 to 93 in bbe7300

	method = getattr(
	self, 'visit_%s' % node['type'], self.default_visit)
	self._method_cache[node_type] = method

...which store references to methods that are bound to self in a member of self.

Possible solution

We worked around the problem by monkey patching ParsedResult so that it (1) caches a default_interpreter for use when options=None, and (2) uses it in search(). If I understand correctly, we could go further and use a global TreeInterpreter for all ParsedResult instances. The TreeInterpreter seems to be stateless apart from self._method_cache and that implementation seems to be thread-safe (with only the risk of multiple lookups for the same method in a multithreaded case).

I'd be happy to contribute a PR for either version if this would be welcome.

How to reproduce

The following reproducer shows the problem:

import jmespath

import gc
gc.set_debug(gc.DEBUG_COLLECTABLE)

pattern = jmespath.compile("foo")
value = {"foo": "bar"}

for _ in range(1000000):
    pattern.search(value)

...where the output contains one million repetitions of something like:

gc: collectable <TreeInterpreter 0x10f634fa0>
gc: collectable <dict 0x10f63e780>
gc: collectable <Options 0x10f634520>
gc: collectable <Functions 0x10f6345b0>
gc: collectable <method 0x10f63ee80>
gc: collectable <dict 0x10f63eb00>

[springcomp]

@mrry we at JMESPath Community are currently working on improving the standard and extending the feature set from JMESPath.

We have a set of useful improvements as our next milestone pretty much nailed down.

Our next step is to implement those specifications in the Python library. As a matter of fact, I'm currently contributing | to | this repository.

Once done, we will either have those PRs accepted here or, most likely, we will implement them in our own fork.

Then we will proceed and submit PRs for inclusion in the AWS CLI and Azure CLI repositories.

That's why I would be very interested in a fix as you mention here.
On behalf of the JMESPath Community, please, feel free to submit a pull request here or in our own fork. Here is better as I think we should always contribute back to this orignal implementation.

[jamesls]

Confirmed. In addition to the GC pressure, it's also the source of a large
number of unnecessary allocations. Updating your repro code with calls to
tracemalloc, I'm seeing the method cache as the primary culprit:

[ Top 10 current snapshot]
jmespath/visitor.py:85: size=67.4 KiB, count=1233, average=56 B

83 class Visitor(object):
84     def __init__(self):
85         self._method_cache = {}

jmespath/visitor.py:93: size=67.4 KiB, count=411, average=168 B

87    def visit(self, node, *args, **kwargs):
88        node_type = node['type']
89        method = self._method_cache.get(node_type)
90        if method is None:
91            method = getattr(
92                self, 'visit_%s' % node['type'], self.default_visit)
93            self._method_cache[node_type] = method

...

Each option has its own tradeoffs to consider:

• Using a single interpreter per parsed result wouldn't solve the case
  for custom options being passed in, and providing a cache
  per unique option class brings its own set of problems.
• Changing the cache to be per-class instead of per-instance would work, but
  the implementation isn't as straightforward as a class level cache because
  we want to account for subclasses of TreeInterpreter.

Other thing to consider was that getattr() was pretty slow in py2 world,
but I've not done any benchmarking on our supported versions of py3. It
might be as simple as removing the cache if there's minimal perf benefits.

At any rate, should be able to address this, just need to work through the
various tradeoffs and see which one makes the most sense. Thanks for reporting.