Skip to content

Commit c90ddaa

Browse files
fangererfangerer
committed
[GR-71615] Enable -O3 and LTO for C API.
PullRequest: graalpython/4144
2 parents 2e52832 + a77c978 commit c90ddaa

13 files changed

Lines changed: 106 additions & 29 deletions

File tree

‎graalpython/com.oracle.graal.python.cext/CMakeLists.txt‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,10 @@ set(TARGET_LIBPYTHON "python-native")
5555
# common variables and compile/link options (for all build targets)
5656
######################################################################
5757

58+
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE TRUE)
59+
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELWITHDEBINFO TRUE)
60+
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_DEBUG FALSE)
61+
5862
if (MSVC)
5963
SET(CMAKE_BUILD_TYPE "RelWithDebInfo" CACHE STRING "" FORCE)
6064
SET(CFLAGS_WARNINGS /Wall /WX
@@ -109,7 +113,6 @@ endif()
109113

110114
# preprocessor defines for all platforms
111115
add_compile_definitions(
112-
NDEBUG
113116
WITH_FREELISTS=1
114117
)
115118

‎graalpython/com.oracle.graal.python.cext/include/internal/pycore_object.h‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -367,9 +367,12 @@ _PyObject_IS_GC(PyObject *obj)
367367
static inline size_t
368368
_PyType_PreHeaderSize(PyTypeObject *tp)
369369
{
370-
// GraalPy change: we put only one pointer for dict, we don't store it inlined [GR-61996]
370+
/* Note: the pre-header size must have a 16-byte alignment. This is
371+
necessary for higher optimization levels which assume the alignment
372+
(inferred from the struct size) and emit instructions that require
373+
this alignment. */
371374
return _PyType_IS_GC(tp) * sizeof(PyGC_Head) +
372-
_PyType_HasFeature(tp, Py_TPFLAGS_PREHEADER) * /* 2 * */ sizeof(PyObject *);
375+
_PyType_HasFeature(tp, Py_TPFLAGS_PREHEADER) * 2 * sizeof(PyObject *);
373376
}
374377

375378
void _PyObject_GC_Link(PyObject *op);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111
#if 0 // GraalPy change
1212
#include "pycore_call.h" // _PyObject_CallNoArgs()
1313
#include "pycore_ceval.h" // _Py_EnterRecursiveCallTstate()
14-
#include "pycore_object.h" // _Py_CheckSlotResult()
1514
#endif // GraalPy change
15+
#include "pycore_object.h" // _Py_CheckSlotResult()
1616
#include "pycore_long.h" // _Py_IsNegative
1717
#include "pycore_pyerrors.h" // _PyErr_Occurred()
1818
#include "pycore_pystate.h" // _PyThreadState_GET()

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

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2882,18 +2882,9 @@ void _Py_SetRefcnt(PyObject* obj, Py_ssize_t cnt) {
28822882
return;
28832883
}
28842884
PyObject *dest;
2885-
if (points_to_py_handle_space(obj))
2886-
{
2885+
if (points_to_py_handle_space(obj)) {
28872886
dest = pointer_to_stub(obj);
2888-
#ifndef NDEBUG
2889-
if (GraalPyPrivate_Debug_CAPI())
2890-
{
2891-
set_PyObject_ob_refcnt(obj, cnt);
2892-
}
2893-
#endif
2894-
}
2895-
else
2896-
{
2887+
} else {
28972888
dest = obj;
28982889
}
28992890
dest->ob_refcnt = cnt;

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,12 +1326,19 @@ PyObject* GraalPyPrivate_Tuple_Alloc(PyTypeObject* type, Py_ssize_t nitems) {
13261326
if (alloc == NULL) {
13271327
return PyErr_NoMemory();
13281328
}
1329+
1330+
// GraalPy change: zero whole object
1331+
memset(alloc, '\0', size + presize);
1332+
13291333
obj = (PyObject *)(alloc + presize);
13301334
if (presize) {
1331-
// GraalPy change: different header layout, no GC link
1332-
((PyObject **)alloc)[0] = NULL;
1335+
// GraalPy change: different header layout
1336+
// ((PyObject **)alloc)[0] = NULL;
1337+
// ((PyObject **)alloc)[1] = NULL;
1338+
_PyObject_GC_Link(obj);
13331339
}
1334-
memset(obj, '\0', size);
1340+
// GraalPy change: whole memory is zero'd above
1341+
// memset(obj, '\0', size);
13351342

13361343
if (type->tp_itemsize == 0) {
13371344
_PyObject_Init(obj, type);

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12823,7 +12823,9 @@ _PyUnicodeWriter_WriteASCIIString(_PyUnicodeWriter *writer,
1282312823
if (len == -1)
1282412824
len = strlen(ascii);
1282512825

12826+
#if 0 // GraalPy change
1282612827
assert(ucs1lib_find_max_char((const Py_UCS1*)ascii, (const Py_UCS1*)ascii + len) < 128);
12828+
#endif // GraalPy change
1282712829

1282812830
if (writer->buffer == NULL && !writer->overallocate) {
1282912831
PyObject *str;

‎graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_gc.py‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,7 @@ def test_native_class(self):
192192
ID_OBJ12 = 10
193193
ID_OBJ13 = 11
194194
ID_OBJ14 = 12
195+
ID_OBJ15 = 13
195196

196197
# don't rely on deterministic Java GC behavior by default on GraalPy
197198
RELY_ON_GC = os.environ.get("RELY_ON_GC", not GRAALPY)
@@ -204,13 +205,15 @@ def test_native_class(self):
204205

205206
if GRAALPY:
206207
get_handle_table_id = __graalpython__.get_handle_table_id
208+
storage_to_native = __graalpython__.storage_to_native
207209
def assert_is_strong(x): assert __graalpython__.is_strong_handle_table_ref(x)
208210
def assert_is_weak(x): assert not __graalpython__.is_strong_handle_table_ref(x)
209211
else:
210212
# just that the test is compatible with CPython
211-
def get_handle_table_id(object): return -1
212-
def assert_is_strong(x): pass
213-
def assert_is_weak(x): pass
213+
def get_handle_table_id(_): return -1
214+
def storage_to_native(_): pass
215+
def assert_is_strong(_): pass
216+
def assert_is_weak(_): pass
214217

215218
class TestGCRefCycles(unittest.TestCase):
216219
def _trigger_gc(self):
@@ -357,6 +360,7 @@ def assert_is_freed(id): pass
357360
obj10 = TestCycle0(ID_OBJ10)
358361
obj11 = TestCycle0(ID_OBJ11)
359362
obj14 = TestCycle0(ID_OBJ14)
363+
obj15 = TestCycle0(ID_OBJ15)
360364

361365
# Legend
362366
# '=>'
@@ -462,6 +466,19 @@ def assert_is_freed(id): pass
462466
htid_l4 = get_handle_table_id(l4)
463467
del l4
464468

469+
# establish cycle: l5 => obj15 =ht=> l5
470+
# update_refs: 11 1
471+
# subtract_refs: 10 0
472+
# move_unreachable: 10 11
473+
# update_refs: 11 11
474+
# subtract_refs: 10 10
475+
# commit_weak_cand: l5 => obj15 =ht-> l5
476+
l5 = [obj15]
477+
storage_to_native(l5)
478+
obj15.set_obj(l5)
479+
htid_l5 = get_handle_table_id(l5)
480+
del obj15
481+
465482
# everything should still be alive
466483
assert_is_alive(ID_OBJ2)
467484
assert_is_alive(ID_OBJ3)
@@ -474,16 +491,19 @@ def assert_is_freed(id): pass
474491
assert_is_alive(ID_OBJ10)
475492
assert_is_alive(ID_OBJ11)
476493
assert_is_alive(ID_OBJ14)
494+
assert_is_alive(ID_OBJ15)
477495
assert_is_strong(htid_l)
478496
assert_is_strong(htid_l1)
479497
assert_is_strong(htid_l2)
480498
assert_is_strong(htid_l3)
481499
assert_is_strong(htid_l4)
500+
assert_is_strong(htid_l5)
482501
assert_is_strong(htid_d0)
483502

484503
del obj2, l, obj3
485504
del obj4, obj5
486505
del obj6, d0, obj7
506+
del l5
487507

488508
####################################### GC #######################################
489509
self._trigger_gc()
@@ -511,6 +531,7 @@ def assert_is_freed(id): pass
511531
assert_is_strong(htid_l1)
512532
assert_is_strong(htid_l4)
513533
assert_is_strong(htid_d0)
534+
assert_is_freed(ID_OBJ15)
514535

515536
rescued_obj4 = l1[0]
516537
del l1

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1555,7 +1555,7 @@ static Object doNative(Object weakCandidates,
15551555
GC_LOGGER.fine(PythonUtils.formatJString("Transitioning to weak reference to break a reference cycle for %s, refcount=%d",
15561556
abstractObjectNativeWrapper.ref, abstractObjectNativeWrapper.getRefCount()));
15571557
}
1558-
updateRefNode.clearStrongRef(inliningTarget, abstractObjectNativeWrapper);
1558+
updateRefNode.clearStrongRefButKeepInGCList(inliningTarget, abstractObjectNativeWrapper);
15591559
}
15601560

15611561
// next = GC_NEXT(gc)

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiGCSupport.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444

4545
import java.util.logging.Level;
4646

47+
import com.oracle.graal.python.PythonLanguage;
4748
import com.oracle.graal.python.builtins.objects.cext.capi.CApiGCSupportFactory.GCListRemoveNodeGen;
4849
import com.oracle.graal.python.builtins.objects.cext.capi.CApiGCSupportFactory.PyObjectGCDelNodeGen;
4950
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.PExternalFunctionWrapper;
@@ -55,6 +56,7 @@
5556
import com.oracle.graal.python.builtins.objects.cext.structs.CStructAccess.FreeNode;
5657
import com.oracle.graal.python.builtins.objects.cext.structs.CStructs;
5758
import com.oracle.graal.python.runtime.PythonContext;
59+
import com.oracle.graal.python.runtime.PythonOptions;
5860
import com.oracle.graal.python.util.PythonUtils;
5961
import com.oracle.truffle.api.dsl.Cached;
6062
import com.oracle.truffle.api.dsl.GenerateCached;
@@ -165,12 +167,24 @@ public static long executeUncached(long opUntagged) {
165167
return GCListRemoveNodeGen.getUncached().execute(null, opUntagged);
166168
}
167169

170+
/**
171+
* Remove the Python object denoted by the given {@code PyObject*} pointer from the GC list
172+
* it is currently contained in. This will apply {@code AS_GC(op)} first.
173+
*/
174+
public final void executeOp(Node inliningTarget, long op) {
175+
if (PythonLanguage.get(inliningTarget).getEngineOption(PythonOptions.PythonGC)) {
176+
execute(inliningTarget, HandlePointerConverter.pointerToStub(op));
177+
}
178+
}
179+
168180
public abstract long execute(Node inliningTarget, long opUntagged);
169181

170182
@Specialization
171183
static long doGeneric(long opUntagged,
172184
@Cached(inline = false) CStructAccess.ReadI64Node readI64Node,
173185
@Cached(inline = false) CStructAccess.WriteLongNode writeLongNode) {
186+
assert PythonLanguage.get(null).getEngineOption(PythonOptions.PythonGC);
187+
174188
// issue a log message before doing the first memory access
175189
if (GC_LOGGER.isLoggable(Level.FINER)) {
176190
GC_LOGGER.finer(PythonUtils.formatJString("attempting to remove 0x%x from GC generation", opUntagged));

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2152,30 +2152,48 @@ static Object doIt(Object object) {
21522152
public abstract static class UpdateStrongRefNode extends Node {
21532153

21542154
public final void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, long refCount) {
2155-
execute(inliningTarget, wrapper, refCount > MANAGED_REFCNT);
2155+
execute(inliningTarget, wrapper, refCount > MANAGED_REFCNT, false);
21562156
}
21572157

2158-
public final void clearStrongRef(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper) {
2159-
execute(inliningTarget, wrapper, false);
2158+
/**
2159+
* Makes the handle table reference of the given wrapper weak but keeps the native object
2160+
* stub in the GC list (if currently contained in any). The only valid use case for this
2161+
* method is when iterating over all objects of a GC list, calling this method on each
2162+
* object and in the end, dropping the whole GC list.
2163+
*/
2164+
public final void clearStrongRefButKeepInGCList(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper) {
2165+
execute(inliningTarget, wrapper, false, true);
21602166
}
21612167

2162-
public abstract void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong);
2168+
public abstract void execute(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong, boolean keepInGcList);
21632169

21642170
@Specialization
2165-
static void doGeneric(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong,
2171+
static void doGeneric(Node inliningTarget, PythonAbstractObjectNativeWrapper wrapper, boolean setStrong, boolean keepInGcList,
21662172
@Cached InlinedConditionProfile hasRefProfile,
2167-
@Cached PyObjectGCTrackNode gcTrackNode) {
2173+
@Cached PyObjectGCTrackNode gcTrackNode,
2174+
@Cached GCListRemoveNode gcListRemoveNode) {
2175+
assert CompilerDirectives.isPartialEvaluationConstant(keepInGcList);
2176+
21682177
PythonObjectReference ref;
21692178
if (hasRefProfile.profile(inliningTarget, (ref = wrapper.ref) != null)) {
21702179
assert ref.pointer == wrapper.getNativePointer();
21712180
if (setStrong && !ref.isStrongReference()) {
21722181
ref.setStrongReference(wrapper);
21732182
if (ref.gc && PythonLanguage.get(inliningTarget).getEngineOption(PythonOptions.PythonGC)) {
21742183
// gc = AS_GC(op)
2175-
long gc = wrapper.getNativePointer() - CStructs.PyGC_Head.size();
2184+
long gc = ref.pointer - CStructs.PyGC_Head.size();
21762185
gcTrackNode.execute(inliningTarget, gc);
21772186
}
21782187
} else if (!setStrong && ref.isStrongReference()) {
2188+
/*
2189+
* As soon as the reference is made weak, we remove it from the GC list because
2190+
* there are ways to iterate a GC list (e.g. 'PyUnstable_GC_VisitObjects') and
2191+
* while doing so, the objects may be accessed. Since weakly referenced objects
2192+
* may die any time, this could lead to dangling pointers being used.
2193+
*/
2194+
if (!keepInGcList && ref.gc) {
2195+
gcListRemoveNode.executeOp(inliningTarget, ref.pointer);
2196+
}
21792197
ref.setStrongReference(null);
21802198
}
21812199
}

0 commit comments

Comments
 (0)