PythonScopeJpyImpl: Disable Conversion Cache #5123

nbauernfeind · 2024-02-08T00:02:53Z

This fixes a memory leak discovered in #5112 via repeated runs of:

import time, jpy
from deephaven import empty_table, garbage_collect

row_count = 10_000_000

begin_time = time.perf_counter_ns()
source = empty_table(row_count).update(["X = repeat(ii % 250, 100)"]) 
empty_table(1).select('Y = 1')
print('Rows / Sec:', row_count / ((time.perf_counter_ns() - begin_time) / 1_000_000_000))

Runtime = jpy.get_type('java.lang.Runtime')
print('Gigs Used Before GC:',
        (Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()) / 1024 / 1024 / 1024)

del source;
del row_count;
del begin_time
for i in range(10):
    garbage_collect()

print('Gigs Used After GC:',
        (Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()) / 1024 / 1024 / 1024)
del Runtime

Nightlies running here: https://github.com/nbauernfeind/deephaven-core/actions/runs/7833860264/

engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java

engine/table/src/test/java/io/deephaven/engine/table/impl/lang/TestQueryLanguageParser.java

engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java

engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java

jmao-denver

weak-values cache makes sense for the main global scope
like the decoupling of QueryScope from QLP

…) for now

engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java

rcaudy · 2024-02-10T01:34:22Z

engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java

+    default Map<String, Object> toMap() {
+        return toMap(entry -> true);
+    }


Isn't this kind of a bad implementation when we could just copy?

engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java

rcaudy · 2024-02-10T01:44:25Z

engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java

@@ -98,8 +94,9 @@ public synchronized void init(TableDefinition tableDefinition) {

        try {
            final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
-            final Map<String, Object> possibleParams = queryScope.toMap();
-            for (Map.Entry<String, Object> param : possibleParams.entrySet()) {
+            final Map<String, Object> queryScopeVariables = queryScope.toMap(


It seems to me like the script session query scope should apply this predicate internally, as the standalone scope guards its input to avoid introducing any bad names. If we did that, then I think we wouldn't need the predicate in toMap().

I'm willing to do this if this is your preference. I was considering the other usages. They both filter to Table instances -- unfortunately they also need to unwrap first, and probably we would prefer to unwrap once.

rcaudy · 2024-02-10T01:49:57Z

engine/table/src/main/java/io/deephaven/engine/table/impl/select/codegen/FormulaAnalyzer.java

            final String columnSuffix = DhFormulaColumn.COLUMN_SUFFIX;
            final Class<?> vectorType = DhFormulaColumn.getVectorType(columnDefinition.getDataType());

            possibleVariables.put(columnDefinition.getName() + columnSuffix, vectorType);
+            columnVariables.add(columnDefinition.getName() + columnSuffix);


Order is opposite from condition filter. Is that bad? It could get confusing if we had column names ending in underscores, regardless.

I think in a later PR, we should combine this phase from ACF and FA into one utility method.

…/QueryLanguageParser.java Co-authored-by: Ryan Caudy <rcaudy@gmail.com>

rcaudy

.

rcaudy · 2024-02-12T16:43:56Z

engine/table/src/main/java/io/deephaven/engine/util/PythonEvaluatorJpy.java

+        // forcefully load this helper class which imports python modules
+        PyCallableWrapperJpyImpl.init();


I'm worried this is some kind of band-aid allowing us to mask a deeper issue.

Integrations/src/main/java/io/deephaven/integrations/python/PythonObjectWrapper.java

engine/table/src/main/java/io/deephaven/engine/util/PyCallableWrapperJpyImpl.java

rcaudy · 2024-02-12T20:02:42Z

server/src/main/java/io/deephaven/server/console/python/PythonImportInitializer.java

+import io.deephaven.engine.util.PyCallableWrapperJpyImpl;
+import io.deephaven.integrations.python.PythonObjectWrapper;
+
+public abstract class PythonImportInitializer {


Feels like this pattern could be replaced with service loader.

I think the problem w/ service loader is that the classes are on the classpath regardless of whether we are running python or groovy. In this current pattern, it's only invoked when we are coming from known-python script session.

engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java

nbauernfeind added bug Something isn't working core Core development tasks python NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Feb 8, 2024

nbauernfeind added this to the 1. January 2024 (end of month) milestone Feb 8, 2024

nbauernfeind requested review from jmao-denver and rcaudy February 8, 2024 00:02

nbauernfeind self-assigned this Feb 8, 2024

PythonScopeJpyImpl: Only Cache the JNI Lookup

fcec5ea

nbauernfeind force-pushed the gh_5112 branch from 536ede7 to fcec5ea Compare February 8, 2024 03:12

nbauernfeind changed the title ~~PythonScopeJpyImpl: Use a WeakHashMap to Allow Converted Values to be Garbage Collected~~ PythonScopeJpyImpl: Only Cache the JNI Lookup Feb 8, 2024

nbauernfeind added 3 commits February 7, 2024 23:14

push params through QLP

bb43c49

Fix QLP Test

d2a86b2

Fix QLP QueryScopeParam's as Object Bad Impl

33053c1

rcaudy reviewed Feb 8, 2024

View reviewed changes

Ryan's feedback

847aa4b

nbauernfeind force-pushed the gh_5112 branch 3 times, most recently from 392bb8c to 900310d Compare February 9, 2024 03:34

Ryan's feedback

f26ab1a

nbauernfeind force-pushed the gh_5112 branch from 900310d to f26ab1a Compare February 9, 2024 03:38

jmao-denver reviewed Feb 9, 2024

View reviewed changes

engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java Outdated Show resolved Hide resolved

jmao-denver previously approved these changes Feb 9, 2024

View reviewed changes

nbauernfeind added 4 commits February 9, 2024 12:48

Jianfeng's Feedback + RM Cache

0bbc54b

BugFixes; remove cache entirely based on performance

1a96cf5

Keep a cache in place for future testing; disabled (which seems ideal…

7f03162

…) for now

spotless + null collector fix

5f5c70d

nbauernfeind dismissed jmao-denver’s stale review via 5f5c70d February 9, 2024 23:57

Personal Review

0297271

Bugfix; missing column variables in dhformulacolumn

f97fbf8

nbauernfeind force-pushed the gh_5112 branch from f833912 to f97fbf8 Compare February 10, 2024 00:44

rcaudy reviewed Feb 10, 2024

View reviewed changes

nbauernfeind and others added 2 commits February 9, 2024 20:17

Update engine/table/src/main/java/io/deephaven/engine/table/impl/lang…

7b86582

…/QueryLanguageParser.java Co-authored-by: Ryan Caudy <rcaudy@gmail.com>

Update engine/table/src/main/java/io/deephaven/engine/table/impl/lang…

1b82a73

…/QueryLanguageParser.java Co-authored-by: Ryan Caudy <rcaudy@gmail.com>

rcaudy reviewed Feb 10, 2024

View reviewed changes

More bug fixes

cae0428

rcaudy reviewed Feb 12, 2024

View reviewed changes

nbauernfeind added 2 commits February 12, 2024 11:57

Force class initialization of static python import calls

516f78d

Move conversion outside of ensureGil

3318474

rcaudy reviewed Feb 12, 2024

View reviewed changes

Simpler Feedback

6efe258

rcaudy approved these changes Feb 12, 2024

View reviewed changes

nbauernfeind changed the title ~~PythonScopeJpyImpl: Only Cache the JNI Lookup~~ PythonScopeJpyImpl: Disable Conversion Cache Feb 12, 2024

nbauernfeind enabled auto-merge (squash) February 12, 2024 21:25

nbauernfeind merged commit 9fcaba7 into deephaven:main Feb 12, 2024
18 checks passed

github-actions bot locked and limited conversation to collaborators Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PythonScopeJpyImpl: Disable Conversion Cache #5123

PythonScopeJpyImpl: Disable Conversion Cache #5123

nbauernfeind commented Feb 8, 2024 •

edited

Loading

jmao-denver left a comment

rcaudy Feb 10, 2024

rcaudy Feb 10, 2024

nbauernfeind Feb 11, 2024

rcaudy Feb 10, 2024

rcaudy left a comment

rcaudy Feb 12, 2024

rcaudy Feb 12, 2024

devinrsmith Feb 12, 2024

		// forcefully load this helper class which imports python modules
		PyCallableWrapperJpyImpl.init();

PythonScopeJpyImpl: Disable Conversion Cache #5123

PythonScopeJpyImpl: Disable Conversion Cache #5123

Conversation

nbauernfeind commented Feb 8, 2024 • edited Loading

jmao-denver left a comment

Choose a reason for hiding this comment

rcaudy Feb 10, 2024

Choose a reason for hiding this comment

rcaudy Feb 10, 2024

Choose a reason for hiding this comment

nbauernfeind Feb 11, 2024

Choose a reason for hiding this comment

rcaudy Feb 10, 2024

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy Feb 12, 2024

Choose a reason for hiding this comment

rcaudy Feb 12, 2024

Choose a reason for hiding this comment

devinrsmith Feb 12, 2024

Choose a reason for hiding this comment

nbauernfeind commented Feb 8, 2024 •

edited

Loading