Skip to content

Commit d7ad467

Browse files
msimacekmsimacek
committed
[GR-56332] Acquire GIL around upcalls
PullRequest: graalpython/3418
2 parents bfd0224 + 15f9ba1 commit d7ad467

File tree

11 files changed

+136
-59
lines changed

11 files changed

+136
-59
lines changed

‎graalpython/com.oracle.graal.python.cext/modules/_sqlite/connection.c‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,10 +199,8 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, PyObject *database,
199199
// Create and configure SQLite database object.
200200
sqlite3 *db;
201201
int rc;
202-
// GraalPy change: workaround for GR-51314, revert to CPython original once fixed
203-
char* cstr = PyBytes_AS_STRING(bytes);
204202
Py_BEGIN_ALLOW_THREADS
205-
rc = sqlite3_open_v2(cstr, &db,
203+
rc = sqlite3_open_v2(PyBytes_AS_STRING(bytes), &db,
206204
SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE |
207205
(uri ? SQLITE_OPEN_URI : 0), NULL);
208206
if (rc == SQLITE_OK) {

‎graalpython/com.oracle.graal.python.cext/src/dictobject.c‎

Lines changed: 57 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2074,7 +2074,6 @@ PyDict_Clear(PyObject *op)
20742074
}
20752075
ASSERT_CONSISTENT(mp);
20762076
}
2077-
#endif // GraalPy change
20782077

20792078
/* Internal version of PyDict_Next that returns a hash value in addition
20802079
* to the key and value.
@@ -2085,30 +2084,65 @@ int
20852084
_PyDict_Next(PyObject *op, Py_ssize_t *ppos, PyObject **pkey,
20862085
PyObject **pvalue, Py_hash_t *phash)
20872086
{
2088-
// GraalPy change: different implementation
2089-
PyObject *tresult = GraalPyTruffleDict_Next(op, *ppos);
2090-
if (tresult == NULL) {
2091-
if(pkey != NULL) {
2092-
*pkey = NULL;
2093-
}
2094-
if(pvalue != NULL) {
2095-
*pvalue = NULL;
2096-
}
2097-
return 0;
2098-
}
2099-
if (pkey != NULL) {
2100-
*pkey = PyTuple_GetItem(tresult, 0);
2101-
}
2102-
if (pvalue != NULL) {
2103-
*pvalue = PyTuple_GetItem(tresult, 1);
2104-
}
2105-
if (phash != NULL) {
2106-
*phash = PyLong_AsSsize_t(PyTuple_GetItem(tresult, 2));
2107-
}
2108-
*ppos = PyLong_AsSsize_t(PyTuple_GetItem(tresult, 3));
2109-
Py_DECREF(tresult);
2087+
Py_ssize_t i;
2088+
PyDictObject *mp;
2089+
PyObject *key, *value;
2090+
Py_hash_t hash;
2091+
2092+
if (!PyDict_Check(op))
2093+
return 0;
2094+
mp = (PyDictObject *)op;
2095+
i = *ppos;
2096+
if (mp->ma_values) {
2097+
assert(mp->ma_used <= SHARED_KEYS_MAX_SIZE);
2098+
if (i < 0 || i >= mp->ma_used)
2099+
return 0;
2100+
int index = get_index_from_order(mp, i);
2101+
value = mp->ma_values->values[index];
2102+
2103+
key = DK_UNICODE_ENTRIES(mp->ma_keys)[index].me_key;
2104+
hash = unicode_get_hash(key);
2105+
assert(value != NULL);
2106+
}
2107+
else {
2108+
Py_ssize_t n = mp->ma_keys->dk_nentries;
2109+
if (i < 0 || i >= n)
2110+
return 0;
2111+
if (DK_IS_UNICODE(mp->ma_keys)) {
2112+
PyDictUnicodeEntry *entry_ptr = &DK_UNICODE_ENTRIES(mp->ma_keys)[i];
2113+
while (i < n && entry_ptr->me_value == NULL) {
2114+
entry_ptr++;
2115+
i++;
2116+
}
2117+
if (i >= n)
2118+
return 0;
2119+
key = entry_ptr->me_key;
2120+
hash = unicode_get_hash(entry_ptr->me_key);
2121+
value = entry_ptr->me_value;
2122+
}
2123+
else {
2124+
PyDictKeyEntry *entry_ptr = &DK_ENTRIES(mp->ma_keys)[i];
2125+
while (i < n && entry_ptr->me_value == NULL) {
2126+
entry_ptr++;
2127+
i++;
2128+
}
2129+
if (i >= n)
2130+
return 0;
2131+
key = entry_ptr->me_key;
2132+
hash = entry_ptr->me_hash;
2133+
value = entry_ptr->me_value;
2134+
}
2135+
}
2136+
*ppos = i+1;
2137+
if (pkey)
2138+
*pkey = key;
2139+
if (pvalue)
2140+
*pvalue = value;
2141+
if (phash)
2142+
*phash = hash;
21102143
return 1;
21112144
}
2145+
#endif // GraalPy change
21122146

21132147
/*
21142148
* Iterate over a dict. Use like so:

‎graalpython/com.oracle.graal.python.processor/src/com/oracle/graal/python/processor/CApiBuiltinsProcessor.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -168,15 +168,17 @@ private static final class CApiBuiltinDesc {
168168
public final String name;
169169
public final VariableElement[] arguments;
170170
public final VariableElement returnType;
171+
public final boolean acquireGil;
171172
public final String call;
172173
public final String factory;
173174
public int id;
174175

175-
public CApiBuiltinDesc(Element origin, String name, VariableElement returnType, VariableElement[] arguments, String call, String factory) {
176+
public CApiBuiltinDesc(Element origin, String name, VariableElement returnType, VariableElement[] arguments, boolean acquireGil, String call, String factory) {
176177
this.origin = origin;
177178
this.name = name;
178179
this.returnType = returnType;
179180
this.arguments = arguments;
181+
this.acquireGil = acquireGil;
180182
this.call = call;
181183
this.factory = factory;
182184
}
@@ -356,11 +358,12 @@ private void addCApiBuiltins(RoundEnvironment re, List<CApiBuiltinDesc> javaBuil
356358
name = builtinName;
357359
}
358360
var ret = findValue(builtin, "ret", VariableElement.class);
361+
boolean acquireGil = findValue(builtin, "acquireGil", Boolean.class);
359362
String call = name(findValue(builtin, "call", VariableElement.class));
360363
// boolean inlined = findValue(builtin, "inlined", Boolean.class);
361364
VariableElement[] args = findValues(builtin, "args", VariableElement.class).toArray(new VariableElement[0]);
362365
if (((TypeElement) element).getQualifiedName().toString().equals("com.oracle.graal.python.builtins.objects.cext.capi.CApiFunction.Dummy")) {
363-
additionalBuiltins.add(new CApiBuiltinDesc(element, builtinName, ret, args, call, null));
366+
additionalBuiltins.add(new CApiBuiltinDesc(element, builtinName, ret, args, acquireGil, call, null));
364367
} else {
365368
if (!isValidReturnType(ret)) {
366369
processingEnv.getMessager().printError(
@@ -387,7 +390,7 @@ private void addCApiBuiltins(RoundEnvironment re, List<CApiBuiltinDesc> javaBuil
387390
genName += "NodeGen";
388391
}
389392
verifyNodeClass(((TypeElement) element), builtin);
390-
javaBuiltins.add(new CApiBuiltinDesc(element, name, ret, args, call, genName));
393+
javaBuiltins.add(new CApiBuiltinDesc(element, name, ret, args, acquireGil, call, genName));
391394
}
392395
}
393396
}
@@ -634,7 +637,7 @@ private void generateBuiltinRegistry(List<CApiBuiltinDesc> javaBuiltins) throws
634637
for (var builtin : javaBuiltins) {
635638
String argString = Arrays.stream(builtin.arguments).map(b -> "ArgDescriptor." + b).collect(Collectors.joining(", "));
636639
lines.add(" public static final CApiBuiltinExecutable " + builtin.name + " = new CApiBuiltinExecutable(\"" + builtin.name + "\", CApiCallPath." + builtin.call + ", ArgDescriptor." +
637-
builtin.returnType + ", new ArgDescriptor[]{" + argString + "}, " + builtin.id + ");");
640+
builtin.returnType + ", new ArgDescriptor[]{" + argString + "}, " + builtin.acquireGil + ", " + builtin.id + ");");
638641
}
639642
lines.add("");
640643
lines.add(" public static final CApiBuiltinExecutable[] builtins = {");

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextBuiltins.java‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@
172172
import com.oracle.graal.python.nodes.object.GetClassNode;
173173
import com.oracle.graal.python.nodes.statement.AbstractImportNode;
174174
import com.oracle.graal.python.nodes.util.CastToJavaIntExactNode;
175+
import com.oracle.graal.python.runtime.GilNode;
175176
import com.oracle.graal.python.runtime.IndirectCallData;
176177
import com.oracle.graal.python.runtime.PosixSupportLibrary;
177178
import com.oracle.graal.python.runtime.PosixSupportLibrary.PosixException;
@@ -584,17 +585,19 @@ public static final class CApiBuiltinExecutable implements TruffleObject {
584585
private final CApiTiming timing;
585586
private final ArgDescriptor ret;
586587
private final ArgDescriptor[] args;
588+
private final boolean acquireGil;
587589
@CompilationFinal private CallTarget callTarget;
588590
private final CApiCallPath call;
589591
private final String name;
590592
private final int id;
591593

592-
public CApiBuiltinExecutable(String name, CApiCallPath call, ArgDescriptor ret, ArgDescriptor[] args, int id) {
594+
public CApiBuiltinExecutable(String name, CApiCallPath call, ArgDescriptor ret, ArgDescriptor[] args, boolean acquireGil, int id) {
593595
this.timing = CApiTiming.create(false, name);
594596
this.name = name;
595597
this.call = call;
596598
this.ret = ret;
597599
this.args = args;
600+
this.acquireGil = acquireGil;
598601
this.id = id;
599602
}
600603

@@ -619,6 +622,10 @@ public String name() {
619622
return name;
620623
}
621624

625+
public boolean acquireGil() {
626+
return acquireGil;
627+
}
628+
622629
CExtToNativeNode createRetNode() {
623630
return ret.createPythonToNativeNode();
624631
}
@@ -773,6 +780,7 @@ public static ExecuteCApiBuiltinNode getUncached(@SuppressWarnings("unused") CAp
773780

774781
static final class CachedExecuteCApiBuiltinNode extends ExecuteCApiBuiltinNode {
775782
private final CApiBuiltinExecutable cachedSelf;
783+
@Child private GilNode gilNode = GilNode.create();
776784
@Child private CExtToNativeNode retNode;
777785
@Children private final CExtToJavaNode[] argNodes;
778786
@Child private CApiBuiltinNode builtinNode;
@@ -788,6 +796,7 @@ static final class CachedExecuteCApiBuiltinNode extends ExecuteCApiBuiltinNode {
788796

789797
@Override
790798
Object execute(CApiBuiltinExecutable self, Object[] arguments) {
799+
boolean wasAcquired = self.acquireGil() && gilNode.acquire();
791800
CApiTiming.enter();
792801
try {
793802
try {
@@ -827,6 +836,7 @@ Object execute(CApiBuiltinExecutable self, Object[] arguments) {
827836
throw CompilerDirectives.shouldNotReachHere("return type while handling PException: " + cachedSelf.getRetDescriptor() + " in " + self.name);
828837
}
829838
} finally {
839+
gilNode.release(wasAcquired);
830840
CApiTiming.exit(self.timing);
831841
}
832842
}
@@ -935,7 +945,11 @@ public enum CApiCallPath {
935945
*/
936946
boolean inlined() default false;
937947

938-
boolean acquiresGIL() default true;
948+
/**
949+
* Whether we need to acquire GIL around the call. Since our conversion nodes currently
950+
* assume GIL, this should be only set to false by GIL-manipulating builtins.
951+
*/
952+
boolean acquireGil() default true;
939953

940954
/**
941955
* @see CApiCallPath

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextCEvalBuiltins.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@
8888

8989
public final class PythonCextCEvalBuiltins {
9090

91-
@CApiBuiltin(ret = PyThreadState, args = {}, acquiresGIL = false, call = Direct)
91+
@CApiBuiltin(ret = PyThreadState, args = {}, acquireGil = false, call = Direct)
9292
abstract static class PyEval_SaveThread extends CApiNullaryBuiltinNode {
9393
private static final TruffleLogger LOGGER = CApiContext.getLogger(PyEval_SaveThread.class);
9494

@@ -102,7 +102,7 @@ static Object save(@Cached GilNode gil) {
102102
}
103103
}
104104

105-
@CApiBuiltin(ret = Void, args = {PyThreadState}, acquiresGIL = false, call = Direct)
105+
@CApiBuiltin(ret = Void, args = {PyThreadState}, acquireGil = false, call = Direct)
106106
abstract static class PyEval_RestoreThread extends CApiUnaryBuiltinNode {
107107
private static final TruffleLogger LOGGER = CApiContext.getLogger(PyEval_RestoreThread.class);
108108

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextDictBuiltins.java‎

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,12 @@
4343
import static com.oracle.graal.python.builtins.PythonBuiltinClassType.AttributeError;
4444
import static com.oracle.graal.python.builtins.PythonBuiltinClassType.SystemError;
4545
import static com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiCallPath.Direct;
46-
import static com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiCallPath.Ignored;
4746
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Int;
47+
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PY_HASH_T_PTR;
48+
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PY_SSIZE_T_PTR;
4849
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObject;
4950
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObjectBorrowed;
51+
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObjectPtr;
5052
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObjectTransfer;
5153
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_hash_t;
5254
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
@@ -59,6 +61,7 @@
5961

6062
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
6163
import com.oracle.graal.python.builtins.modules.BuiltinConstructors.StrNode;
64+
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApi5BuiltinNode;
6265
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiBinaryBuiltinNode;
6366
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiBuiltin;
6467
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiNullaryBuiltinNode;
@@ -67,6 +70,8 @@
6770
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiUnaryBuiltinNode;
6871
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.PromoteBorrowedValue;
6972
import com.oracle.graal.python.builtins.objects.PNone;
73+
import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions;
74+
import com.oracle.graal.python.builtins.objects.cext.structs.CStructAccess;
7075
import com.oracle.graal.python.builtins.objects.common.EconomicMapStorage;
7176
import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes.SetItemNode;
7277
import com.oracle.graal.python.builtins.objects.common.HashingStorage;
@@ -107,6 +112,8 @@
107112
import com.oracle.truffle.api.dsl.Cached.Shared;
108113
import com.oracle.truffle.api.dsl.Fallback;
109114
import com.oracle.truffle.api.dsl.Specialization;
115+
import com.oracle.truffle.api.interop.InteropLibrary;
116+
import com.oracle.truffle.api.library.CachedLibrary;
110117
import com.oracle.truffle.api.nodes.Node;
111118
import com.oracle.truffle.api.profiles.InlinedBranchProfile;
112119
import com.oracle.truffle.api.profiles.InlinedLoopConditionProfile;
@@ -122,12 +129,17 @@ static Object run(@Cached PythonObjectFactory factory) {
122129
}
123130
}
124131

125-
@CApiBuiltin(ret = PyObjectTransfer, args = {PyObject, Py_ssize_t}, call = Ignored)
126-
abstract static class PyTruffleDict_Next extends CApiBinaryBuiltinNode {
132+
@CApiBuiltin(ret = Int, args = {PyObject, PY_SSIZE_T_PTR, PyObjectPtr, PyObjectPtr, PY_HASH_T_PTR}, call = Direct)
133+
abstract static class _PyDict_Next extends CApi5BuiltinNode {
127134

128135
@Specialization
129-
static Object run(PDict dict, long pos,
136+
static int next(PDict dict, Object posPtr, Object keyPtr, Object valuePtr, Object hashPtr,
130137
@Bind("this") Node inliningTarget,
138+
@CachedLibrary(limit = "2") InteropLibrary lib,
139+
@Cached CStructAccess.ReadI64Node readI64Node,
140+
@Cached CStructAccess.WriteLongNode writeLongNode,
141+
@Cached CStructAccess.WritePointerNode writePointerNode,
142+
@Cached CApiTransitions.PythonToNativeNode toNativeNode,
131143
@Cached InlinedBranchProfile needsRewriteProfile,
132144
@Cached InlinedBranchProfile economicMapProfile,
133145
@Cached HashingStorageLen lenNode,
@@ -138,14 +150,14 @@ static Object run(PDict dict, long pos,
138150
@Cached HashingStorageIteratorKeyHash itKeyHash,
139151
@Cached PromoteBorrowedValue promoteKeyNode,
140152
@Cached PromoteBorrowedValue promoteValueNode,
141-
@Cached HashingStorageSetItem setItem,
142-
@Cached PythonObjectFactory factory) {
153+
@Cached HashingStorageSetItem setItem) {
143154
/*
144155
* We need to promote primitive values and strings to object types for borrowing to work
145156
* correctly. This is very hard to do mid-iteration, so we do all the promotion for the
146157
* whole dict at once in the first call (which is required to start with position 0). In
147158
* order to not violate the ordering, we construct a completely new storage.
148159
*/
160+
long pos = readI64Node.read(posPtr);
149161
if (pos == 0) {
150162
HashingStorage storage = dict.getDictStorage();
151163
int len = lenNode.execute(inliningTarget, storage);
@@ -200,20 +212,33 @@ static Object run(PDict dict, long pos,
200212
it.setState((int) pos - 1);
201213
boolean hasNext = itNext.execute(inliningTarget, storage, it);
202214
if (!hasNext) {
203-
return getNativeNull(inliningTarget);
215+
return 0;
216+
}
217+
long newPos = it.getState() + 1;
218+
writeLongNode.write(posPtr, newPos);
219+
if (!lib.isNull(keyPtr)) {
220+
Object key = itKey.execute(inliningTarget, storage, it);
221+
assert promoteKeyNode.execute(inliningTarget, key) == null;
222+
// Borrowed reference
223+
writePointerNode.write(keyPtr, toNativeNode.execute(key));
224+
}
225+
if (!lib.isNull(valuePtr)) {
226+
Object value = itValue.execute(inliningTarget, storage, it);
227+
assert promoteValueNode.execute(inliningTarget, value) == null;
228+
// Borrowed reference
229+
writePointerNode.write(valuePtr, toNativeNode.execute(value));
204230
}
205-
Object key = itKey.execute(inliningTarget, storage, it);
206-
Object value = itValue.execute(inliningTarget, storage, it);
207-
assert promoteKeyNode.execute(inliningTarget, key) == null;
208-
assert promoteValueNode.execute(inliningTarget, value) == null;
209-
long hash = itKeyHash.execute(inliningTarget, storage, it);
210-
int newPos = it.getState() + 1;
211-
return factory.createTuple(new Object[]{key, value, hash, newPos});
231+
if (!lib.isNull(hashPtr)) {
232+
long hash = itKeyHash.execute(inliningTarget, storage, it);
233+
writeLongNode.write(hashPtr, hash);
234+
}
235+
return 1;
212236
}
213237

214238
@Fallback
215-
Object run(@SuppressWarnings("unused") Object dict, @SuppressWarnings("unused") Object pos) {
216-
return getNativeNull();
239+
@SuppressWarnings("unused")
240+
static int run(Object dict, Object posPtr, Object keyPtr, Object valuePtr, Object hashPtr) {
241+
return 0;
217242
}
218243
}
219244

0 commit comments

Comments
 (0)