Den mån 28 juli 2025 kl 15:19 skrev Yasuhito FUTATSUKI < futat...@yf.bsdclub.org>:
> 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> > > Agree that it seems we will have to chase Python's future optimisations and we will probably have to expect future breakage. I trust your judgement on the best way to test and verify this. Kind regards, Daniel
