Issue2230

classification
Title: Jython evaluation blocks under heavy load with high multi-core systems
Type: Severity: normal
Components: Core Versions: Jython 2.7, Jython 2.5
Milestone: Jython 2.7.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amak, jeff.allen, wdroste, zyasoft
Priority: normal Keywords: patch

Created on 2014-11-14.18:36:33 by wdroste, last changed 2020-02-01.09:01:55 by jeff.allen.

Files
File name Uploaded Description Edit Remove
jython.diff wdroste, 2014-11-14.18:36:33 Patch to fix blocking
Messages
msg9209 (view) Author: William M Droste (wdroste) Date: 2014-11-14.18:36:32
Basically the PyType class is caching lookups but under 60k/s evaluations on 24 core machines it causes a great deal of contention so much so it locks the application. Using the high-scale-lib the lock contention is removed and tests show 80k/s evals.
msg9219 (view) Author: Alan Kennedy (amak) Date: 2014-11-21.17:31:28
Is there any way that you could reduce this to a simple code fragment that illustrates the problem?
msg9237 (view) Author: Jim Baker (zyasoft) Date: 2014-12-15.18:36:27
Makes sense. Let's see if we can apply post beta 4, in time for the RC.
msg9264 (view) Author: Jim Baker (zyasoft) Date: 2014-12-23.16:50:14
I looked into this requested change. The problem is that NonBlockingIdentityHashMap does not support weak keys/weak values, which is necessary to prevent memory leaks.
msg9314 (view) Author: Jim Baker (zyasoft) Date: 2015-01-06.18:04:42
Perhaps the best solution here is to allow the user to provide their own instance for classToBuilder, while maintaining the current behavior as default.

Such a change would be done at the Java integration level, vs the current convenience wrapper of the Jython bash script.
msg9622 (view) Author: Jim Baker (zyasoft) Date: 2015-03-11.21:37:30
Putting what I suggested (msg9314) on the backlog for 2.7.1 - we should offer more tuning opportunities for Jython, and supporting it is an easy win.
msg9623 (view) Author: Jim Baker (zyasoft) Date: 2015-03-11.21:50:00
William, one thing: did you try changing the concurrency level in MapMaker? We are using the default, which is 4, which may explain the lock contention you are seeing. If you haven't, could you try running your performance tests with a higher level?

Per the docs, "a significantly lower value can lead to thread contention"
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect/MapMaker.html#concurrencyLevel(int)

So you might want to try something like

    public static synchronized PyType fromClass(Class<?> c, boolean hardRef) {
        if (class_to_type == null) {
            class_to_type = new MapMaker().concurrencyLevel(16).weakKeys().weakValues().makeMap();
            addFromClass(PyType.class, null);
        }

We might want to make this the default regardless, given that this is a singleton.
msg9632 (view) Author: William M Droste (wdroste) Date: 2015-03-12.00:02:23
I'll give it a shot, there was several issues.

1. Stacktraces in jython are very expensive.. so logging was deceptively contributing to slow execution.

2. The code we used built a javax.script.CompiledScript for code isolation.. but it ended up being slow so we switch to creating functions and using invoke..

I'll need to go back and see if that solves other issues.. but the main issue is is this code in our environment was being executed at a very high rate... I think I've reduced that rate and therefore its not as big of a deal.

..
As far as previous comments about the solution path.. I choose NonBlockingIdentityHashMap because .class objects should already be in perm gen and this would simply store references so it, should be a pretty small memory footprint basically just pointers.. and while weakkeys allows one to unload low volume references.. IHMO the footprint was so small that the application probably has bigger issues.. a bit extra memory for holding the references.. there was no discernible difference in our application.
msg9636 (view) Author: Jim Baker (zyasoft) Date: 2015-03-12.05:18:42
William, thanks for taking a look at this. Re your points:

1. Stack traces in Java are expensive when compared to simply using exceptions. But we are certainly making this be even more expensive in Py#getStackTrace, which is simply ancient code in Jython that parses the stack trace string representation, instead of using http://docs.oracle.com/javase/7/docs/api/java/lang/Throwable.html#getStackTrace(), available since Java 1.4. (hg blame shows that this code still has parts unchanged from when Jim Huginin and Barry Warsaw worked on it 16-17 years ago!)

2. If at all possible, you want to use code objects that are already compiled and invoke them, vs recompiling every single time. Basically every time you eval/compile something on Jython, no matter how small, it creates a class and loads it with a classloader. So I think this is the right strategy, even if it means we don't have such a good performance test :)

3. Re weak references - this is certainly true of your scenario, however, it causes memory leaks for other usage. So this seems to be the best compromise. Providing a pluggable map for class_to_type would be even better of course.
msg9637 (view) Author: Jim Baker (zyasoft) Date: 2015-03-12.16:33:56
Tracebacks are even more expensive than I thought. If you look at PyTraceback.java, you can see that PyTraceback#getLine does not even attempt to cache the line, or optimize access to the file by getting multiple lines.

So this means we should at least port Lib/linecache.py to Java to speed things up (https://docs.python.org/2/library/linecache.html), as well as avoid a dependency in Python code for something as critical as the traceback.
msg9638 (view) Author: William M Droste (wdroste) Date: 2015-03-12.16:47:29
On point 2

I was caching the CompiledScript and using Bindings to pass in variables. I didn't realize it still required re-compiling or additional processing.

The use case was for user provided expressions..

Example:
x AND y

My assessment was dynamically wrapping the expression in a function could potentially be error prone as the language syntax depends on line spacing.

However the performance difference was so great that I ended up doing it anyway.. and sure enough there were some sample expressions I had to modify but it wasn't as bad as I anticipated.

So I'm dynamically turning the expression above into..

def fx(x,y):
 return x AND y
msg9639 (view) Author: William M Droste (wdroste) Date: 2015-03-12.16:49:26
Addendum: 

I found out this was also true for other scripting languages, Groovy in particular.
msg9640 (view) Author: Jim Baker (zyasoft) Date: 2015-03-12.17:12:13
Ack. CPython has been compiled into Python bytecode since the earliest release, nearly 25 years ago; and this has also been true of Jython (to Java bytecode), IronPython (to CLR), and PyPy (to Python bytecode, then JITed). Of popular JVM languages, I believe only JRuby attempts to do a classical AST interpreter (but does do a tiered compilation strategy for hot code).
msg9916 (view) Author: Jim Baker (zyasoft) Date: 2015-04-20.21:04:19
Revisit tuning, possibly providing some user configuration, per msg9623
msg11602 (view) Author: Jim Baker (zyasoft) Date: 2017-09-25.03:40:40
Based on the diff from William, the problem is being addressed by Jeff Allen's fix here: https://bitbucket.org/tournesol/jython-fgtype/

We expect this fix to be included in 2.7.2
msg11754 (view) Author: Jeff Allen (jeff.allen) Date: 2018-03-06.20:10:43
PyType has changed a lot in how it addresses the issues discussed here. It would be useful to know whether the current tip, or subsequent 2.7.2b runs in the environment described. But that was 3 years ago ...

Leaving tagged at 2.7.2, hopefully.
msg11854 (view) Author: Jeff Allen (jeff.allen) Date: 2018-03-25.17:17:30
Assuming the OP's original analysis is correct, that contention for the cache in PyType is at the root of this, we expect the switch to ClassValue (https://hg.python.org/jython/rev/55756721fa02) to have resolved this issue at scale.

2.7.2 beta may provide experience with something like the hardware described here, to confirm or disprove this optimism.
msg12960 (view) Author: Jeff Allen (jeff.allen) Date: 2020-02-01.09:01:55
Given time elapsed and feedback on #2834, I think this is properly fixed.
History
Date User Action Args
2020-02-01 09:01:55jeff.allensetstatus: pending -> closed
messages: + msg12960
2018-03-25 17:17:31jeff.allensetstatus: open -> pending
resolution: accepted -> fixed
messages: + msg11854
versions: + Jython 2.7
2018-03-06 20:10:44jeff.allensetnosy: + jeff.allen
messages: + msg11754
2017-09-25 03:40:40zyasoftsetmessages: + msg11602
2015-12-29 23:54:53zyasoftsetmilestone: Jython 2.7.1 -> Jython 2.7.2
2015-12-28 18:35:38zyasoftsetpriority: high -> normal
2015-04-20 21:04:19zyasoftsetmessages: + msg9916
milestone: Jython 2.7.1
2015-03-12 17:12:14zyasoftsetmessages: + msg9640
2015-03-12 16:49:26wdrostesetmessages: + msg9639
2015-03-12 16:47:29wdrostesetmessages: + msg9638
2015-03-12 16:33:57zyasoftsetmessages: + msg9637
2015-03-12 05:18:43zyasoftsetmessages: + msg9636
2015-03-12 00:02:23wdrostesetmessages: + msg9632
2015-03-11 21:50:01zyasoftsetmessages: + msg9623
2015-03-11 21:37:31zyasoftsetpriority: high
resolution: accepted
messages: + msg9622
2015-01-06 18:04:43zyasoftsetmessages: + msg9314
2014-12-23 16:50:14zyasoftsetmessages: + msg9264
2014-12-15 18:36:27zyasoftsetnosy: + zyasoft
messages: + msg9237
2014-11-21 17:31:28amaksetnosy: + amak
messages: + msg9219
2014-11-14 18:36:41wdrostesetversions: + Jython 2.5
2014-11-14 18:36:34wdrostecreate