Skip to content

Commit

Permalink
Add a config param to avoid Python exception checking overheads (#26573)
Browse files Browse the repository at this point in the history
Adds `Python.checkExceptions` (on by default), which allows users to
turn off the exception checking to avoid overhead.

This PR also includes a few other cleanups and adds a future for
#26579

- [x] `start_test test/library/packages/Python` 
- [x] `start_test test/library/packages/Python --compopts
-scheckExceptions=false`
- this has 1 expected failure of `argPassingTest`, which tests
exceptions

Future work:
- It would be nice to support this as a per-interpreter param as well,
but that opens us up to issues with generics and classes. So for now
thats a todo/nice-to-have

[Reviewed by @DanilaFe]
  • Loading branch information
jabraham17 authored Jan 23, 2025
2 parents fc62d44 + 183368b commit 506b25e
Show file tree
Hide file tree
Showing 12 changed files with 112 additions and 15 deletions.
54 changes: 39 additions & 15 deletions modules/packages/Python.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,28 @@ module Python {
private use CWChar;
private use OS.POSIX only getenv;

/*
Use 'objgraph' to detect memory leaks in the Python code. Care should be
taken when interpreting the output of this flag, not all memory leaks are
under Chapel's control. For example, printing a Python list leaks memory
according to 'objgraph'. Furthermore, some memory is still held when until
the interpreter is closed, like the module import cache.
*/
config const pyMemLeaks = false;

/*
Check for exceptions after each Python API call. This is important for
correctness, but may have a performance impact.
.. warning::
While turning this flag off may improve performance, it may also lead to
segmentation faults and other undefined behavior. This flag should only
be turned off if you are certain that no exceptions will be thrown by
the Python code, or if a hard crash is acceptable.
*/
config param checkExceptions = true;

// TODO: this must be first to avoid use-before-def, but that makes it first in the docs
// is there a way to avoid this?
/* Represents the python NoneType */
Expand All @@ -246,14 +266,6 @@ module Python {
/* Represents the python value 'None' */
const None = new NoneType();


@chpldoc.nodoc
enum codeKind {
source,
bytecode,
pickle
}

private proc getOsPathSepHelper(interp: borrowed Interpreter): string throws {
var os = PyImport_ImportModule("os");
interp.checkException();
Expand All @@ -276,6 +288,18 @@ module Python {
Do not create more than one instance of this class.
*/
class Interpreter {

// TODO: add ability to override the global checkExceptions
// currently blocked by https://github.com/chapel-lang/chapel/issues/26579
/*
Whether to check for exceptions after each Python API call. This is
important for correctness, but may have a performance impact.
See :config:`Python.checkExceptions` and
:method:`Interpreter.checkException` for more information.
*/
// param checkExceptions: bool = Python.checkExceptions;

@chpldoc.nodoc
var converters: List.list(owned TypeConverter);
@chpldoc.nodoc
Expand Down Expand Up @@ -553,9 +577,9 @@ module Python {
most users should not need to call this method directly.
*/
inline proc checkException() throws {
var exc = chpl_PyErr_GetRaisedException();
if exc {
throw PythonException.build(this, exc);
if Python.checkExceptions {
var exc = chpl_PyErr_GetRaisedException();
if exc then throw PythonException.build(this, exc);
}
}

Expand Down Expand Up @@ -1056,7 +1080,7 @@ module Python {
Py_DECREF(this.obj);
}

proc check() throws do this.interpreter.checkException();
inline proc check() throws do this.interpreter.checkException();

/*
Returns the :type:`~CTypes.c_ptr` to the underlying Python object.
Expand Down Expand Up @@ -1227,7 +1251,7 @@ module Python {
*/
proc init(interpreter: borrowed Interpreter,
in modName: string, in moduleContents: string) throws {
super.init(interpreter, nil: PyObjectPtr);
super.init(interpreter, nil: PyObjectPtr, isOwned=true);
this.modName = modName;
init this;
this.obj = interpreter.compileModule(moduleContents, modName);
Expand All @@ -1238,7 +1262,7 @@ module Python {
*/
proc init(interpreter: borrowed Interpreter,
in modName: string, in moduleBytecode: bytes) throws {
super.init(interpreter, nil: PyObjectPtr);
super.init(interpreter, nil: PyObjectPtr, isOwned=true);
this.modName = modName;
init this;
this.obj = interpreter.compileModule(moduleBytecode, modName);
Expand All @@ -1264,7 +1288,7 @@ module Python {
this.fnName = fnName;
}
proc init(interpreter: borrowed Interpreter, in lambdaFn: string) throws {
super.init(interpreter, nil:PyObjectPtr, isOwned=true);
super.init(interpreter, nil: PyObjectPtr, isOwned=true);
this.fnName = "<anon>";
init this;
this.obj = interpreter.compileLambda(lambdaFn);
Expand Down
8 changes: 8 additions & 0 deletions test/classes/generic/nonGenericSubclassGenericSuperclass.bad
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
nonGenericSubclassGenericSuperclass.chpl:19: internal error: RES-FUN-ION-2796 chpl version 2.4.0 pre-release (9ab0d960101)
Note: This source location is a guess.

Internal errors indicate a bug in the Chapel compiler,
and we're sorry for the hassle. We would appreciate your reporting this bug --
please see https://chapel-lang.org/bugs.html for instructions. In the meantime,
the filename + line number above may be useful in working around the issue.

19 changes: 19 additions & 0 deletions test/classes/generic/nonGenericSubclassGenericSuperclass.chpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class Generic {
param x = 1;
}

class Parent {
var x: borrowed Generic(?);
proc init(x: borrowed Generic(?)) {
this.x = x;
}
}

class Child: Parent(?) {
proc init(x: borrowed Generic(?)) {
super.init(x);
}
}

var g = new Generic();
var c = new Child(g);
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bug: inheriting from a generic class in a non-generic subclass
#26579
Empty file.
4 changes: 4 additions & 0 deletions test/library/packages/Python/correctness/noExceptions.bad
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
noExceptions.chpl:18: error: unresolved call 'Interpreter.init(0)'
$CHPL_HOME/modules/packages/Python.chpl:nnnn: note: this candidate did not match: Interpreter.init()
noExceptions.chpl:18: note: because call includes 1 argument
$CHPL_HOME/modules/packages/Python.chpl:nnnn: note: but function can only accept 0 arguments
32 changes: 32 additions & 0 deletions test/library/packages/Python/correctness/noExceptions.chpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
use Python;

var hello = """
print('Hello, World!')
""";
var hello_raise = """
raise Exception('Hello, World!')
""";

// use default checking
{
var interp = new Interpreter();
var mod = new Module(interp, 'hello', hello);
}

// explicitly use no checking, blocked by https://github.com/chapel-lang/chapel/issues/26579
{
var interp = new Interpreter(false);
var mod = new Module(interp, 'hello', hello);
}

// explicitly use checking, blocked by https://github.com/chapel-lang/chapel/issues/26579
{
var interp = new Interpreter(true);
try {
var mod = new Module(interp, 'hello', hello_raise);
} catch e: PythonException {
writeln("Caught exception: ", e.message());
} catch {
writeln("Caught unknown exception");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-scheckExceptions=false
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
feature request: ability to set override the global checkExceptions
3 changes: 3 additions & 0 deletions test/library/packages/Python/correctness/noExceptions.good
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Hello, World!
Hello, World!
Caught exception: Hello, World!
3 changes: 3 additions & 0 deletions test/library/packages/Python/correctness/noExceptions.prediff
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/usr/bin/env bash

$CHPL_HOME/util/test/prediff-obscure-module-linenos $@
Empty file.

0 comments on commit 506b25e

Please sign in to comment.