perf: memoize __getattr__ via LRU cache (review-iteration on #14, towards upstream #596)#17
Open
Tagar wants to merge 4 commits into
Open
perf: memoize __getattr__ via LRU cache (review-iteration on #14, towards upstream #596)#17Tagar wants to merge 4 commits into
Tagar wants to merge 4 commits into
Conversation
…4j#557) Cold-start work, C1 of the issue py4j#557 series. Today every attribute walk along ``gateway.jvm.X.Y.Z.method`` re-issues one reflection round-trip per level — JVMView, JavaPackage, and JavaClass all have ``__getattr__`` methods that call out to the JVM without caching the result. After the first walk, the JVM-side answer is stable for the JVM's lifetime, so the next 999 walks pay the same round-trip cost for no new information. This change adds a per-instance dict cache on each of: * ``JVMView._attr_cache`` — top-level names resolved on ``.jvm.X`` * ``JavaPackage._attr_cache`` — names resolved on ``.X.Y`` * ``JavaClass._members`` — static members on ``.System.currentTimeMillis`` Only successful resolutions are cached. Misses (Py4JError) keep raising so that a later ``java_import`` or classpath change can still surface a previously-missing class. The ``UserHelpAutoCompletion`` sentinel path and direct return values on ``JavaClass.__getattr__`` are NOT cached because they each have caller-visible semantics that caching would change. Thread safety mirrors the existing ``_statics`` / ``_dir_sequence_and_cache`` convention: plain dict assignments are atomic under CPython's GIL, worst case is double-fill with the same value. The existing comments in those caches explicitly accept this tradeoff. Local measurement on the new XA-3 scenario (M-series + JDK 21): before: ~94 ms / 1000 walks = ~94 us per walk (3 reflection RTTs) after: ~269 us / 1000 walks = ~0.27 us per walk (dict get) Approx 350x speed-up on the steady-state attribute-walk path; the first walk in any chain still pays the full reflection cost. X1-1 unchanged (10 000 currentTimeMillis() calls, ~232 ms either way). Co-authored-by: Isaac
Merging this PR will improve performance by ×34
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Tagar
force-pushed
the
c1-bounded-attr-cache
branch
2 times, most recently
from
d8e065c to
0145d0d
Compare
…changelog Iterates on the original cache change in this PR based on review feedback. Cache shape ----------- * Replaces the unbounded ``dict`` cache with a bounded LRU (``_BoundedAttrCache``, default ``maxsize = 1024`` per instance) using ``OrderedDict.move_to_end`` + ``popitem(last=False)``. * Bound: 1 024 entries — generous enough that real-world workloads never evict (PySpark sessions touch ~50-200 unique names per instance), but caps pathological iteration like ``for name in many_classes: gateway.jvm[name]``. * ``get``/``put`` chain two ``OrderedDict`` ops; a concurrent eviction between them can leave the second op operating on a missing key. Both methods catch ``KeyError`` — the races are benign (``get`` already has the correct value; ``put`` already inserted) and the lost LRU promotion / eviction is recovered on the next op. GC-safety contract (only non-finalizable types are cached) ---------------------------------------------------------- * The cache stores **only** ``JavaClass``, ``JavaPackage``, and static-member ``JavaMember`` instances. None of these register a ``ThreadSafeFinalizer`` weakref or hold a JVM-allocated ``target_id``; their ``target_id``s are either absent or synthetic (``STATIC_PREFIX + fqn``). * The one ``JavaClass.__getattr__`` branch that could return a ``JavaObject`` (static field constants) deliberately bypasses the cache. ``JavaObject`` is the only py4j type with JVM-side finalization, and it stays out of every cache. * This is the contract that keeps PySpark ML's ``JavaWrapper.__del__`` -> ``gateway.detach(self._java_obj)`` machinery unaffected (SPARK-18274 / SPARK-50959 / SPARK-51880 scenarios all unaffected). Shutdown defensive clearing --------------------------- * ``JavaGateway.shutdown()`` clears the default ``self.jvm``'s attribute cache in a ``finally`` block so cached ``JavaClass`` / ``JavaPackage`` references no longer pin ``_gateway_client`` past shutdown — even on the ``raise_exception=True`` path. * Secondary ``JVMView`` instances created via ``new_jvm_view()`` are caller-managed — they die with the caller's reference and their cache is reclaimed naturally, consistent with how other in-file caches (``JavaObject._methods``, ``JavaClass._statics``, ``JVMView._dir_sequence_and_cache``) rely on their parent's lifecycle. Identity stability documented ----------------------------- * Each of ``JavaClass`` / ``JavaPackage`` / ``JVMView`` gains a ``.. note::`` block on its docstring explaining: repeated accesses to the same name typically return the *same* instance (was a fresh instance every time); identity may flip again once the cache evicts the entry; the GC-safety contract; and the ``java_import``-overlap caveat (re-importing a different FQN under the same simple name does not invalidate prior cache entries — use ``new_jvm_view`` if you need that semantics). Tests ----- Unit (``py4j/tests/attr_cache_test.py``, new file, 8 tests): * Basic semantics: ``test_get_miss_returns_none``, ``test_put_then_get_returns_value``, ``test_put_replaces_existing_value`` * LRU semantics: ``test_eviction_drops_oldest``, ``test_get_promotes_to_most_recent``, ``test_put_existing_promotes`` * Concurrency: ``test_concurrent_get_put_does_not_raise`` (4-thread race against maxsize=10), ``test_concurrent_clear_during_get_put_does_not_raise`` (4 readers + 1 writer + 1 clearer — covers the ``shutdown()._cache.clear()`` race) Integration (``MemoryManagementTest``, +7 tests): * ``testAttrCacheDoesNotAffectJavaObjectDetach`` — walks the cache, constructs two ``JavaObject`` instances, detaches them, asserts the ``ThreadSafeFinalizer`` registry delta is zero. Direct proof the cache does not pin JVM-side bindings. * ``testConcurrentColdFqnWalk`` — 16-thread Barrier-synchronized cold-walk on the same FQN. * ``testAttrCacheSurvivesShutdownMidWalk`` — clean error semantics post-shutdown. * ``testTwoGatewaysDoNotShareAttrCache`` — per-instance isolation. * ``testNoCycleInCachedInstances`` — ``weakref.ref(gateway)() is None`` after ``gc.collect()``. * ``testMissesAreNotCached`` — exercises ``System.noSuchMember``; asserts ``JavaClass._members`` size unchanged after the ``Py4JError``. * ``testShutdownRaiseExceptionStillClearsCache`` — forces the gateway-client shutdown to raise, calls ``shutdown(raise_exception=True)``, asserts the cache is cleared via the ``finally`` clause. Integration (``GarbageCollectionTest`` in ``client_server_test.py``, +1 test): * ``testAttrCacheInClientServerMode`` — pinned-thread mode coverage; 8 worker threads + main thread converge on the same ``JavaClass`` instance. Changelog --------- * ``py4j-web/changelog.rst`` gains an "Unreleased" bullet documenting the cache, the identity-semantics shift, the GC-safety contract, and the shutdown-clearing behavior. Cites issue py4j#128 (closes) and issue py4j#557 (refs). Validation ---------- Local: 19/19 attr-cache + MemoryManagementTest tests pass on M-series + JDK 21. Co-authored-by: Isaac
…test caplog Regression introduced by the previous follow-up commit on this PR (elevation of ``logger.info`` to ``logger.warning`` in ``JavaGateway.shutdown()``): ``memory_leak_test::ClientServerTest`` started failing with ``AssertionError: 5 != 1`` on the matrix CI cells — finalization-counting tests saw only 1 of 5 expected JavaGateway-tree objects reach ``__del__``. Mechanism (verified end-to-end): 1. ``InstrClientServer`` constructs ``InstrJavaClient`` without passing ``finalizer_deque`` (default ``None``) — see ``py4j-python/src/py4j/tests/instrumented.py:148``. 2. ``clientserver.shutdown()`` calls ``JavaClient.shutdown_gateway()`` which has ``self.finalizer_deque.appendleft(SHUTDOWN_FINALIZER_WORKER)`` in a ``finally`` block — raises ``AttributeError: 'NoneType' object has no attribute 'appendleft'`` (``clientserver.py:253``). 3. ``JavaGateway.shutdown()`` outer ``except Exception`` catches it; with ``raise_exception=False`` (the test's default) it logs at WARNING with ``exc_info=True``. 4. pytest's ``caplog`` fixture captures records at WARNING level and above by default. The captured ``LogRecord`` retains the full ``exc_info = (exc_type, exc_value, traceback)``. 5. The ``traceback``'s ``tb_frame`` chain holds the current frame's locals — including ``self``, which is the ``JavaGateway`` / ``ClientServer`` instance and therefore the root of the entire object graph the test is trying to finalize. 6. As long as the test holds the captured ``LogRecord`` (i.e. until the test ends), the ``ClientServer`` tree (``_gateway_client``, ``_callback_server``, ``gateway_property``, ``ClientServerConnection``, ``PythonServer``, etc.) is reachable and cannot be GC'd. 7. The test's ``assertEqual(5, len(FINALIZED))`` sees only the ``InstrumentedObject`` finalize (the one object the test doesn't hold via locals). Below WARNING the record is discarded immediately by ``caplog``, so locals are released and the tree finalizes as expected. Fix: * ``shutdown()`` outer except: ``logger.warning`` -> ``logger.info`` (matches the pre-PR level — same diagnostic visibility for users running at INFO+, no caplog pin). * ``shutdown()`` cache-clear cleanup except: ``logger.warning`` -> ``logger.debug``. Same reasoning; debug is below caplog default. Not changed: * ``_BoundedAttrCache.get`` / ``put`` ``KeyError`` handlers KEEP ``logger.warning``. Those calls log only the ``name`` string (no ``exc_info=True``) — the captured ``LogRecord`` doesn't hold a frame containing ``self=JavaGateway``, so no graph pin. Baunsgaard's observability ask is still honored where it matters. Local verification: 7/7 ``ClientServerTest`` PASS with this fix on M-series + JDK 21 (was 6/7 fail with the prior commit). Co-authored-by: Isaac
6 tasks
Tagar
force-pushed
the
c1-bounded-attr-cache
branch
from
9489618 to
eaea497
Compare
Two follow-ups on this PR.
(1) clientserver.py — fix latent bug in JavaClient.shutdown_gateway and
JavaClient.garbage_collect_object that surfaced via the test
failures CI flagged.
InstrJavaClient (and any external subclass) can construct without
propagating ``finalizer_deque``, leaving it at its default ``None``.
Both ``shutdown_gateway`` (line 253) and ``garbage_collect_object``
(line 234) unconditionally call ``self.finalizer_deque.appendleft(...)``
— raising ``AttributeError: 'NoneType' object has no attribute
'appendleft'`` on the None deque. Guard both call sites: only
enqueue if ``finalizer_deque is not None``; otherwise fall back to
the synchronous super-class path (``garbage_collect_object``) or
skip the SHUTDOWN sentinel (``shutdown_gateway``) because there is
no worker thread to signal.
(2) java_gateway.py — restore @Baunsgaard's logger.warning ask while
avoiding the pytest-caplog test regression.
The previous revert to logger.info was a symptom-silencer. The real
fix: log at WARNING with the exception's class and ``str(e)`` as
explicit message args, NOT ``exc_info=True``. Mechanism:
- ``logger.warning(..., exc_info=True)`` produces a LogRecord that
retains the exception's traceback. pytest's caplog captures
records at WARNING+ by default; the captured traceback's
``tb_frame`` chain holds the current frame's locals — including
``self`` (the ``JavaGateway`` / ``ClientServer``) — for the
test's lifetime. That pin prevents finalization-counting tests
(``memory_leak_test::ClientServerTest``) from reaching their
expected ``len(FINALIZED) == 5`` assertion.
- Logging the exception's class name + message as string args
keeps WARNING-level diagnostic visibility but produces a
LogRecord whose msg arguments are plain strings — no frame
references, no pin.
Both shutdown sites updated: the outer ``except`` in
``JavaGateway.shutdown()`` and the cache-clear ``except`` in the
same method's ``finally`` block.
Result: Baunsgaard's WARNING level ask is now honored everywhere
he flagged it, AND ``memory_leak_test::ClientServerTest`` stays
green (the regression that motivated the prior logger.info revert
is structurally fixed at the test-pin layer, not papered over).
Co-authored-by: Isaac
Tagar
force-pushed
the
c1-bounded-attr-cache
branch
from
eaea497 to
d00c042
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Memoize
__getattr__onJVMView,JavaPackage, andJavaClassso repeated walks along an FQN chain likegateway.jvm.java.lang.System.currentTimeMillis()collapse from N reflection round-trips down to dict lookups after the first walk.Iteration on #14 — same caching shape but bounded LRU + GC-safety hardening, identity-stability docs, defensive shutdown clearing, and integration regression test.
What this PR is (and isn't)
The cache stores only non-finalizable Python metadata:
JavaClass— pure reflective handle, FQN string + gateway client refsJavaPackage— sameJavaMember(static, fromJavaClass.__getattr__) —target_id = "z:" + fqnis synthetic, never an allocated JVM bindingJavaObjectis never cached.JavaObjectis the only py4j class that registers aThreadSafeFinalizerweakref and triggers_garbage_collect_objecton Python-side reclamation. The oneJavaClass.__getattr__branch that could return aJavaObject(static field constants) deliberately bypasses the cache. Verified in code (java_gateway.pyelse-arm) and in a new regression test.Diff vs #14
_BoundedAttrCache(OrderedDict + move_to_end + popitem)wrapper. Cap = 1024 per instance — generous enough that real-world workloads never evict, capped so pathological iteration (for name in many_classes: gateway.jvm[name]) can't grow without bound.get()/put(): each method chains twoOrderedDictops (lookup +move_to_end, or insert +popitem). A concurrent eviction between the two ops could leave the second one operating on a missing key →KeyError. Caught withtry/except; both races are benign._BoundedAttrCache: only types with no JVM-side finalization are stored.JavaObjectis explicitly out of scope.java_importoverlap docstrings onJavaClass,JavaPackage,JVMView.JavaGateway.shutdown()now clearsgateway.jvm._attr_cacheso cachedJavaClass/JavaPackagereferences no longer pin_gateway_clientpast shutdown. Nested caches die naturally with their parents. This is strictly stricter than the precedent (JavaObject._methods/JavaClass._statics/JVMView._dir_sequence_and_cachedo not clear on shutdown).attr_cache_test.py— 7 unit tests covering miss, hit, replace, LRU eviction order, promotion on hit + on re-put, and concurrent-race smoke under 4 threads.MemoryManagementTest.testAttrCacheDoesNotAffectJavaObjectDetach— walksgateway.jvm.java.lang.StringBuffer(populates cache), constructs two instances, detaches them, assertsThreadSafeFinalizer.finalizersdelta is zero. Direct proof that the cache doesn't pin JVM-side bindings.Why 1024
launch_gateway()scriptsjava_importof an entire packageLocal measurement (M-series + JDK 21)
Test plan
attr_cache,finalizer,protocol,signals)MemoryManagementTestintegration tests pass, including the newtestAttrCacheDoesNotAffectJavaObjectDetach