`TreeInterpreter` creates reference cycle, causing GC pressure
mrry opened this issue · 2 comments
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
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
...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>
@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.
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 ofTreeInterpreter
.
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.