Hello,
On 2025/07/28 19:03, Daniel Sahlberg wrote:
> Den sön 27 juli 2025 kl 04:55 skrev Yasuhito FUTATSUKI <
> futat...@yf.bsdclub.org>:
>
>> Hello,
>>
>> On 2025/07/27 8:22, Daniel Sahlberg wrote:
>>
>>> I'm not very fond of the idea of just disabling the tests under 3.14,
>>> chances are we'll never come to re-writing to weakrefs and then we've
>>> basically lost a test.
>>
>> We've already lost a test on under Python >= 3.14, if we cannot explain
>> that the test is correct. I think such a test tests nothing.
>>
>> @Yasuhito FUTATSUKI <futat...@yf.bsdclub.org> Do I understand it correctly
>>> that you are concerned why we can keep the existing expected count ...
>>>
>>> 434: for baton in editor.batons:
>>> 435: self.assertEqual(sys.getrefcount(baton[2]), 2,
>>> ^ HERE
>>> 436: "leak on baton %s after replay without errors"
>>> 437: % repr(baton))
>>> 438: del e_baton
>>>
>>> ... but we have to modify it ...
>>>
>>> 439: self.assertEqual(sys.getrefcount(e_ptr), expected,
>>> ^ HERE
>>> 440: "leak on editor baton after replay without
>>> errors")
>> Briefly, no. I worry about our test using sys.getrefcount() does not
>> work as we expected under modified reference counting.
>>
>
> Isn't the issue at hand here that the interpreter - sometimes - borrow
> argument already on the stack (and thus not changes refcount)? That
> "sometimes" is not very well documented, the best I can find is in the
> PR[1]:
>
> "when we can be sure that the reference in the frame outlives the reference
> that is pushed onto the operand stack"
>> This seems to affect the call to sys.getrefcount(). The cases when refcount
> is modified involve swig-generated classes (libsvn.core.svn_merge_range_t
> and libsvn.delta.svn_delta_editor_t), but I don't know if that helps
> explaining.
Yes, I think it is correct explanation of the comment before sys.getrefcount(r)
in test_mergeinfo_leakage__incorrect_range_t_refcounts(). However, it seems
this is only the first step for introducion of deferred reference counting
(PEP 703[1]). So if we want to keep using reference count for testing,
we should trace how the Python interpreter deal with it, for each modification
for farther optimization.
> It seems like others[2] have had to make similar changes as in r1926575.
I don't think it is right reason that others do it also, of course we
can learn much from them.
>>> Can you outline your idea for using weakrefs?
>>
>> Just collecting weakrefs of the target objects and check them before
>> and after removing those objects.
>>
>
> "collecting" as in the BatonCollector class? But the difference in refcount
> is on the e_ptr object which are not collected as far as I can see. Do I
> miss something?
No, batons generated by for each callbacks in svn_repos_replay().
They are used only in svn_repos_replay C API if callback
Python functions does not holds them to elsewhere. BatonCollector
object provides such callback functions, collecting batons
in BatonCollector.batons list. To check weakrefs, it should
be collected separately.
I also one more try to fix test_replay_batons_refcount() still
using sys.getrefcount() by detecting expected reference count
at runtime, like the code below (not completed yet):
[[[
--- subversion/bindings/swig/python/tests/repository.py (revision 1927470)
+++ subversion/bindings/swig/python/tests/repository.py (working copy)
@@ -87,15 +87,34 @@ class DumpStreamParser(repos.ParseFns3):
class BatonCollector(repos.ChangeCollector):
"""A ChangeCollector with collecting batons, too"""
+
def __init__(self, fs_ptr, root, pool=None, notify_cb=None):
+
+ def get_expected_baton_refcount():
+ """determine expected refcount of batons within a batoun_tuple,
+ by using dumy object"""
+ def make_dummy_baton_tuple(dummy_list):
+ """create a dummy object which has same structure as baton_tuples"""
+ bt = [] # new object without reference from others
+ dummy_list.append((bt,))
+ return bt
+
+ dl = []
+ make_dummy_baton_tuple(dl)
+ for baton in dl:
+ rc = sys.getrefcount(baton[0])
+ break
+ return rc
+
repos.ChangeCollector.__init__(self, fs_ptr, root, pool, notify_cb)
self.batons = []
self.close_called = False
self.abort_called = False
+ self.expected_baton_refcount = get_expected_baton_refcount()
def open_root(self, base_revision, dir_pool=None):
bt = repos.ChangeCollector.open_root(self, base_revision, dir_pool)
- self.batons.append((b'dir baton', b'', bt, sys.getrefcount(bt)))
+ self.batons.append((b'dir baton', b'', bt, self.expected_baton_refcount))
return bt
def add_directory(self, path, parent_baton,
(and more hunks replace get.refcount(bt) to self.expected_baton_refcount)
...
@@ -429,19 +448,22 @@ class SubversionRepositoryTestCase(unittest.TestCa
root = fs.revision_root(self.fs, self.rev)
editor = BatonCollector(self.fs, root)
e_ptr, e_baton = delta.make_editor(editor)
- expected = 1 if sys.version_info >= (3, 14) else 2
+ refcount_at_first = sys.getrefcount(e_ptr)
repos.replay(root, e_ptr, e_baton)
- for baton in editor.batons:
- self.assertEqual(sys.getrefcount(baton[2]), 2,
+ for baton_tuple in editor.batons:
+ # baton_tuple: 4-tuple(baton_type: bytes, node: bytes, bt: baton,
+ # expected_refcount_of_bt: int)
+ self.assertEqual(sys.getrefcount(baton_tuple[2]), baton_tuple[3],
"leak on baton %s after replay without errors"
- % repr(baton))
+ % repr(baton_tuple))
del e_baton
- self.assertEqual(sys.getrefcount(e_ptr), expected,
+ self.assertEqual(sys.getrefcount(e_ptr), refcount_at_first,
"leak on editor baton after replay without errors")
editor = BatonCollectorErrorOnClose(self.fs, root,
error_path=b'branches/v1x')
e_ptr, e_baton = delta.make_editor(editor)
+ refcount_at_first = sys.getrefcount(e_ptr)
self.assertRaises(SubversionException, repos.replay, root, e_ptr, e_baton)
batons = editor.batons
# As svn_repos_replay calls neither close_edit callback nor abort_edit
(same changes on error exit case using BatonCollectorErrorOnClose)
]]]
(I'd thought as Python callback functions are executed by another
Python interpreter sharing baton objects than main interpreter,
those baton reference count may be affected by biased reference
counting. But after returned from callback there is no other
interpreter, I suspect the reference count is merged. So now
I think biased reference counting does not affect the test code
running on main interpreter)
For e_ptr in test_replay_batons_refcounts(), I think it suffice to
compare the reference counts on just after created, and after
calling repos.replay (and doing GC), even if reference counting
might be modified for optimization, as far as the life time of
object is based on reference counting.
For test_mergeinfo_leakage__incorrect_range_t_refcounts(), there already
alternative test for memory leak of svn_merge_range_t object,
test_mergeinfo_leakage__lingering_range_t_objects_after_del().
(I've try to rewrite test_mergeinfo_leakage__incorrect_range_t_refcounts()
using weakrefs, but checking weakrefs after removing mergeinfo
is almost same thing done in
test_mergeinfo_leakage__lingering_range_t_objects_after_del())
> I would argue that the tests are still reasonably valid even if we have to
> modify refcount as long as we only change between Python 3.13 and 3.14 (and
> everything else is the same). If we at one point change the code and miss
> an INCREF/DECREF that should show up in the test cases as long as we stick
> to the same "known good" Python version.
I said "..., if we cannot explain that the test is correct." :)
[1] PEP 703
https://peps.python.org/pep-0703/
Cheers,
--
Yasuhito FUTATSUKI <futat...@yf.bsdclub.org>
