Issue2559
Created on 2017-02-28.21:05:33 by jamesmudd, last changed 2017-03-28.05:30:33 by zyasoft.
Messages | |||
---|---|---|---|
msg11144 (view) | Author: James Mudd (jamesmudd) | Date: 2017-02-28.21:05:32 | |
When running the regression tests on Ubuntu 16.04 with Java version 1.8.0_121 the test_marshal test fails. The error is below. [exec] test test_marshal failed -- multiple errors occurred; run in verbose mode for details The cause of the failure is a StackOverflowError this can be fixed by setting the ThreadStackSize for the JVM larger. I found 3m to be enough so pass -Xss3m to the JVM and the test passes. As this is a test specifically for this i'm not clear that increasing the ThreadStackSize is the best solution or how that option should be added to the tests. Maybe this should be resolved for 2.7.1? |
|||
msg11146 (view) | Author: James Mudd (jamesmudd) | Date: 2017-02-28.22:30:44 | |
Just a bit more info on testing this james@james-ubuntu:~/git/jython$ java -jar dist/jython.jar dist/Lib/test/test_marshal.py -v test_bool (__main__.IntTestCase) ... ok test_int64 (__main__.IntTestCase) ... ok test_ints (__main__.IntTestCase) ... ok test_floats (__main__.FloatTestCase) ... ok test_string (__main__.StringTestCase) ... ok test_unicode (__main__.StringTestCase) ... ok test_dict (__main__.ContainerTestCase) ... ok test_list (__main__.ContainerTestCase) ... ok test_sets (__main__.ContainerTestCase) ... ok test_tuple (__main__.ContainerTestCase) ... ok test_exceptions (__main__.ExceptionTestCase) ... ok test_bug_5888452 (__main__.BugsTestCase) ... ok test_fuzz (__main__.BugsTestCase) ... ok test_loads_recursion (__main__.BugsTestCase) ... ERROR test_patch_873224 (__main__.BugsTestCase) ... ok test_recursion_limit (__main__.BugsTestCase) ... ERROR test_version_argument (__main__.BugsTestCase) ... ok ====================================================================== ERROR: test_loads_recursion (__main__.BugsTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "dist/Lib/test/test_marshal.py", line 232, in test_loads_recursion self.assertRaises(ValueError, marshal.loads, s) File "/home/james/git/jython/dist/Lib/unittest/case.py", line 476, in assertRaises callableObj(*args, **kwargs) File "/home/james/git/jython/dist/Lib/marshal.py", line 28, in loads return load(f) File "/home/james/git/jython/dist/Lib/marshal.py", line 19, in load return u.load() RuntimeError: maximum recursion depth exceeded (Java StackOverflowError) ====================================================================== ERROR: test_recursion_limit (__main__.BugsTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "dist/Lib/test/test_marshal.py", line 244, in test_recursion_limit data = marshal.dumps(head) File "/home/james/git/jython/dist/Lib/marshal.py", line 23, in dumps dump(x, f, version) File "/home/james/git/jython/dist/Lib/marshal.py", line 12, in dump Marshaller(f, version).dump(x) RuntimeError: maximum recursion depth exceeded (Java StackOverflowError) ---------------------------------------------------------------------- Ran 17 tests in 0.335s FAILED (errors=2) Traceback (most recent call last): File "dist/Lib/test/test_marshal.py", line 264, in <module> test_main() File "dist/Lib/test/test_marshal.py", line 255, in test_main test_support.run_unittest(IntTestCase, File "/home/james/git/jython/dist/Lib/test/test_support.py", line 1320, in run_unittest _run_suite(suite) File "/home/james/git/jython/dist/Lib/test/test_support.py", line 1303, in _run_suite raise TestFailed(err) test.test_support.TestFailed: multiple errors occurred james@james-ubuntu:~/git/jython$ But with -Xss3m james@james-ubuntu:~/git/jython$ java -Xss3m -jar dist/jython.jar dist/Lib/test/test_marshal.py -v test_bool (__main__.IntTestCase) ... ok test_int64 (__main__.IntTestCase) ... ok test_ints (__main__.IntTestCase) ... ok test_floats (__main__.FloatTestCase) ... ok test_string (__main__.StringTestCase) ... ok test_unicode (__main__.StringTestCase) ... ok test_dict (__main__.ContainerTestCase) ... ok test_list (__main__.ContainerTestCase) ... ok test_sets (__main__.ContainerTestCase) ... ok test_tuple (__main__.ContainerTestCase) ... ok test_exceptions (__main__.ExceptionTestCase) ... ok test_bug_5888452 (__main__.BugsTestCase) ... ok test_fuzz (__main__.BugsTestCase) ... ok test_loads_recursion (__main__.BugsTestCase) ... ok test_patch_873224 (__main__.BugsTestCase) ... ok test_recursion_limit (__main__.BugsTestCase) ... ok test_version_argument (__main__.BugsTestCase) ... ok ---------------------------------------------------------------------- Ran 17 tests in 0.305s OK james@james-ubuntu:~/git/jython$ If the point of the test is to check for infinite recurrsion then increasing Xss should be ok if the point is to check the stack doesn't get to big then maybe its not a good solution. A few Jython launch scripts specify -Xss1m so it was considered at some point... |
|||
msg11147 (view) | Author: Jim Baker (zyasoft) | Date: 2017-02-28.23:35:54 | |
Sounds like the best alternative is to increase the stack size, given that this is a recursion test after all. Presumably what we are seeing differences in how much stack allocations cost between different OSes, architectures, and Java implementations. There's no standard here that we can rely on that states exactly how much stack allocation some set of ops will cost for Java, so we have to be conservative and ask for more than we might expect. |
|||
msg11153 (view) | Author: James Mudd (jamesmudd) | Date: 2017-03-01.18:53:30 | |
I have a pull request to fix this https://github.com/jythontools/jython/pull/61 It sets a Xss significantly higher than required so it should be sufficient in all cases. |
|||
msg11163 (view) | Author: Stefan Richthofer (stefan.richthofer) | Date: 2017-03-03.12:40:45 | |
James: Your fix in https://github.com/jythontools/jython/pull/61 only addresses Linux et al. The issue would persist on Windows. jython.exe is compiled from jython.py, so an equivalent modification in jython.py would be needed. However, I am not sure if this ought to be fixed in the launcher. Do we want Jython to start with such an increased stacksize always? Just because one regrtest depends on it..? Maybe we should rather adjust regrtest target in build.xml. Or as a compromise we could set the default launcher to 3m and adjust build.xml to run tests with 5m. |
|||
msg11171 (view) | Author: Jim Baker (zyasoft) | Date: 2017-03-04.00:14:02 | |
There is no need to fix this change in the launcher, given that this is just for a specific test at the regrtest level. Users can also specify the desired stack size they need, via -J options; or by directly using Java (eg java -jar jython.jar...). |
|||
msg11172 (view) | Author: Jeff Allen (jeff.allen) | Date: 2017-03-04.09:13:03 | |
I would argue: 1. If we don't expect the test to pass under standard conditions as written, then we should fix the test. 2. If we believe the test demands no more than users can legitimately expect from Jython, then we should fix the behaviour (on all platforms). I tend to think 2 applies in this case. A particular expectation (2000) is acknowledged in _marshal.java, and I think the test should match it (roughly as the comment says, but not marshal.c). This looks relevant: http://bugs.python.org/issue22734, but I suspect the motivation is specific to C-on-Windows, not Windows per se. With Java I think we can expect uniformity cross-platform. |
|||
msg11175 (view) | Author: James Mudd (jamesmudd) | Date: 2017-03-04.21:54:24 | |
Thanks for the comments, I completely forgot to test this on Windows and yes that needs to be sorted out. I tried changing the Xss setting in the python launch script but that didn't work on windows the JVM still had Xss=1024k. Looking a bit more at this I realise this is because the jython.exe is actually built from the jython.py script somehow I couldn't figure out how, maybe using PyInstaller? Does that need to be done on be Windows? I would be happy to do this if someone can give me some hints to the process. Initially I though the exe just called the script. On the other points, I think this should be changed in the launchers (Unix + Windows). If people run Jython by clicking the exe I think should run in a state where the unit tests could pass. Additionally running the regrtest's on Windows using the jython_regrtest.bat script relies on the launcher. So it need to work there, if the unit tests on Windows are to pass (at least if this script is used). Additionally i'm not sure what the advantage to fixing it in the regrtest, but not in the launcher would be? We would save 2mb of ram? Finally what value of Xss would be the best to use? In the pull request I changed to 5m but after a bit more thought I think this is probably more than enough, and would now propose to change to 3m. I know this is a bit arbitrary (pick a number >3) but It would seem reasonable to keep it as small as needed for the tests to pass on every platform (that we can reasonably test). |
|||
msg11176 (view) | Author: Jim Baker (zyasoft) | Date: 2017-03-04.22:09:46 | |
We use PyInstaller; see http://bugs.jython.org/issue1491 for the details on how this was done. It should be automated although IIRC this requires building on Windows. The default stack size is set by the JAVA_STACK env var, which in turn defaults to -Xss1024k Re stack size: this is for each thread, so it can add up. However, we no longer use the default for heap, since it's too low for Jython; it's set to 512M; we may want to increase stack size by default as well to somewhat higher. See https://github.com/jythontools/jython/blob/master/src/shell/jython.py#L211 |
|||
msg11179 (view) | Author: Stefan Richthofer (stefan.richthofer) | Date: 2017-03-04.23:26:00 | |
As follows we can retrieve the Xss value at runtime. Jython 2.7.1rc1 (, Mär 4 2017, 22:31:12) [OpenJDK 64-Bit Server VM (Oracle Corporation)] on java1.8.0_121 Type "help", "copyright", "credits" or "license" for more information. >>> from java.lang.management import ManagementFactory >>> rtmx = ManagementFactory.getRuntimeMXBean() >>> rtmx sun.management.RuntimeImpl@64f555e7 >>> ags = rtmx.getInputArguments() >>> ags [-Xmx512m, -Xss1024k, -Dpython.home=dist, -Dpython.executable=dist/bin/jython] >>> ags[1] u'-Xss1024k' >>> Based on this we could implement a @skipif guarding the case that Xss is <3m. Parsing of the -Xss value should better done on Java-side in a utility method, e.g. in Py.java or so. Also one must consider to properly map the k, m or whatever properly. I know, this method only reveals -Xss if that arg was given on command line. However since our launchers always provide it, this would work reliably on Jython. Should still have a fallback (to None?) if the value wasn't provided, e.g. because launched directly via java-command. How should None then be treated by the @skipif? Actually I don't care. Either way is fine; someone running tests not using a launcher should know what he is doing. |
|||
msg11186 (view) | Author: James Mudd (jamesmudd) | Date: 2017-03-05.18:58:10 | |
I have updated the pull request to fix this https://github.com/jythontools/jython/pull/61 It now sets Xss to 2560k on both Unix and Windows. I think this is a good compromise, on Ubuntu test_marshal fails with 2m but passes with 3m. It actually needs some where around 2100k so 2560k seems like a reasonable value while staying as small as possible. I have rebuilt the windows launcher and although it seems to work for me I would be happier if a few others could try this out as well. |
|||
msg11196 (view) | Author: James Mudd (jamesmudd) | Date: 2017-03-06.22:59:03 | |
Have just updated the pull request again so now the test is updated as well |
|||
msg11217 (view) | Author: Jeff Allen (jeff.allen) | Date: 2017-03-11.17:25:29 | |
Committed at https://hg.python.org/jython/rev/17e40de9a541 |
History | |||
---|---|---|---|
Date | User | Action | Args |
2017-03-28 05:30:33 | zyasoft | set | status: pending -> closed |
2017-03-11 17:25:29 | jeff.allen | set | keywords:
+ test failure causes status: open -> pending resolution: fixed messages: + msg11217 |
2017-03-06 22:59:03 | jamesmudd | set | messages: + msg11196 |
2017-03-05 18:58:11 | jamesmudd | set | messages: + msg11186 |
2017-03-04 23:26:00 | stefan.richthofer | set | messages: + msg11179 |
2017-03-04 22:09:46 | zyasoft | set | messages: + msg11176 |
2017-03-04 21:54:25 | jamesmudd | set | messages: + msg11175 |
2017-03-04 09:13:04 | jeff.allen | set | nosy:
+ jeff.allen messages: + msg11172 |
2017-03-04 00:14:02 | zyasoft | set | messages: + msg11171 |
2017-03-03 12:40:45 | stefan.richthofer | set | priority: normal nosy: + stefan.richthofer messages: + msg11163 components: + Library versions: + Jython 2.7 |
2017-03-01 18:53:30 | jamesmudd | set | messages: + msg11153 |
2017-02-28 23:35:54 | zyasoft | set | nosy:
+ zyasoft messages: + msg11147 |
2017-02-28 22:30:45 | jamesmudd | set | messages: + msg11146 |
2017-02-28 21:05:33 | jamesmudd | create |
Supported by Python Software Foundation,
Powered by Roundup