You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Sahlberg <da...@gmail.com> on 2025/07/23 19:08:48 UTC
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Hi,
The patch below has been floating in dev@ for about a month. It looks looks
like an improvement to me, but it is way out of my comfort zone.
@Joe Orton <jo...@redhat.com> are you able to test this?
Cheers,
Daniel
Den tors 19 juni 2025 kl 14:06 skrev Yasuhito FUTATSUKI <
futatuki@yf.bsdclub.org>:
> Hello,
>
> On 2025/06/19 20:30, Branko Čibej wrote:
> > On 19. 6. 25 12:42, Joe Orton wrote:
> >> On Thu, Jun 19, 2025 at 10:26:43AM +0200, Branko Čibej wrote:
> >>> On 19. 6. 25 10:22,jorton@apache.org wrote:
> >>>> Author: jorton
> >>>> Date: Thu Jun 19 08:22:56 2025
> >>>> New Revision: 1926575
> >>>>
> >>>> URL:http://svn.apache.org/viewvc?rev=1926575&view=rev
> >>>> Log:
> >>>> Fix SWIG test cases for Python 3.14.
> >>>>
> >>>> The interpreter internally avoids some reference count modifications
> when
> >>>> loading objects onto the operands stack by borrowing references when
> >>>> possible. This can lead to smaller reference count values compared to
> >>>> previous Python versions.
> >>>>
> >>>> https://docs.python.org/3.14/whatsnew/3.14.html#whatsnew314-refcount
> >>> How come all the other getrefcount()s in repository.py aren't affected?
> >> I have no idea, sorry, but all the tests pass under 3.14 with this
> >> applied.
> >
> > Yes, I verified that (3.14.0b3 via pyenv). Well, as long as it works ...
>
> Then, I've tweak repository.py without using magic number of refcount
> and adding comments. Although I didn't test it on Python 3.14, but
> it is expected to work.
>
> --
> Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
>
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2025/08/05 17:45, Joe Orton wrote:
> On Tue, Aug 05, 2025 at 02:28:11AM +0900, Yasuhito FUTATSUKI wrote:
>> Hello,
>>
>> Here is a new patch for tests of Python bindings, tests of referene
>> count handling without using fixed literal value for expected reference
>> count value checked by sys.getrefcount() as possible. One test will be
>> skipped if the Python interpreter uses deferred reference counting, but
>> for the skipped test, new test is added for checking reference counting
>> for same objects, by using weakrefs.
>>
>> I've checked it on Python 3.14.0rc1 on FreeBSD.
>
> +1, that works for me too with the Fedora Rawhide Python 3.14rc build as
> well. Thanks again for looking at this!
Thank you for the confirmation.
I also checked that it still works on Python 2.7 (i.e. it can
backport to 1.14.x), and then committed in r1927715.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Aug 05, 2025 at 02:28:11AM +0900, Yasuhito FUTATSUKI wrote:
> Hello,
>
> Here is a new patch for tests of Python bindings, tests of referene
> count handling without using fixed literal value for expected reference
> count value checked by sys.getrefcount() as possible. One test will be
> skipped if the Python interpreter uses deferred reference counting, but
> for the skipped test, new test is added for checking reference counting
> for same objects, by using weakrefs.
>
> I've checked it on Python 3.14.0rc1 on FreeBSD.
+1, that works for me too with the Fedora Rawhide Python 3.14rc build as
well. Thanks again for looking at this!
Regards, Joe
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
Hello,
Here is a new patch for tests of Python bindings, tests of referene
count handling without using fixed literal value for expected reference
count value checked by sys.getrefcount() as possible. One test will be
skipped if the Python interpreter uses deferred reference counting, but
for the skipped test, new test is added for checking reference counting
for same objects, by using weakrefs.
I've checked it on Python 3.14.0rc1 on FreeBSD.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 28 juli 2025 kl 15:19 skrev Yasuhito FUTATSUKI <
futatuki@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 <
> > futatuki@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 <fu...@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 <fu...@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
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Yasuhito FUTATSUKI <fu...@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 <
> futatuki@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 <fu...@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 <fu...@yf.bsdclub.org>
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Den sön 27 juli 2025 kl 04:55 skrev Yasuhito FUTATSUKI <
futatuki@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 <fu...@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.
It seems like others[2] have had to make similar changes as in r1926575.
>
> > 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?
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.
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
>
>
Cheers,
Daniel
[1] https://github.com/python/cpython/pull/130708
[2] https://github.com/python-greenlet/greenlet/pull/442/files
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Yasuhito FUTATSUKI <fu...@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 <fu...@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.
> Can you outline your idea for using weakrefs?
Just collecting weakrefs of the target objects and check them before
and after removing those objects.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Den lör 26 juli 2025 kl 07:22 skrev Yasuhito FUTATSUKI <
futatuki@yf.bsdclub.org>:
> Hello,
>
> On 2025/07/25 22:36, Joe Orton wrote:
> > On Fri, Jul 25, 2025 at 12:37:56AM +0900, Yasuhito FUTATSUKI wrote:
> >> Thank you for testing.
> >>
> >> The intention of those tests are detection of extra reference
> >> remaining after calling callback functions. However, with deferred
> >> reference counting and biased reference counting, it cannot work
> >> as I(we?) expected. So I don't think the changes on r1926575 is
> >> correct if no one cannot explain why this part of the test could
> >> pass without modifying expected refcount.
> >>
> >> I propose:
> >>
> >> * Use weakref for the tests of this purpose
> >> * Until those test is rewrited with using weakref, simply skip
> >> them on Python >= 3.14.
> >
> > Thanks for looking at this, that seems reasonable to me. The attached
> > reverts r1926575 and adds skipIf decorators, does it look correct? I'll
> > run it through a build too.
>
> The patch looks good. Also, I've run check-swig-py on Python 3.11 and
> it did not skip those tests as we expected.
>
> Thanks,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
>
>
Hi,
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.
@Yasuhito FUTATSUKI <fu...@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")
Can you outline your idea for using weakrefs?
Cheers,
Daniel
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
Hello,
On 2025/07/25 22:36, Joe Orton wrote:
> On Fri, Jul 25, 2025 at 12:37:56AM +0900, Yasuhito FUTATSUKI wrote:
>> Thank you for testing.
>>
>> The intention of those tests are detection of extra reference
>> remaining after calling callback functions. However, with deferred
>> reference counting and biased reference counting, it cannot work
>> as I(we?) expected. So I don't think the changes on r1926575 is
>> correct if no one cannot explain why this part of the test could
>> pass without modifying expected refcount.
>>
>> I propose:
>>
>> * Use weakref for the tests of this purpose
>> * Until those test is rewrited with using weakref, simply skip
>> them on Python >= 3.14.
>
> Thanks for looking at this, that seems reasonable to me. The attached
> reverts r1926575 and adds skipIf decorators, does it look correct? I'll
> run it through a build too.
The patch looks good. Also, I've run check-swig-py on Python 3.11 and
it did not skip those tests as we expected.
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Jul 25, 2025 at 12:37:56AM +0900, Yasuhito FUTATSUKI wrote:
> Thank you for testing.
>
> The intention of those tests are detection of extra reference
> remaining after calling callback functions. However, with deferred
> reference counting and biased reference counting, it cannot work
> as I(we?) expected. So I don't think the changes on r1926575 is
> correct if no one cannot explain why this part of the test could
> pass without modifying expected refcount.
>
> I propose:
>
> * Use weakref for the tests of this purpose
> * Until those test is rewrited with using weakref, simply skip
> them on Python >= 3.14.
Thanks for looking at this, that seems reasonable to me. The attached
reverts r1926575 and adds skipIf decorators, does it look correct? I'll
run it through a build too.
Regards, Joe
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
Hello,
On 2025/07/24 20:51, Joe Orton wrote:
> On Thu, Jul 24, 2025 at 11:41:20AM +0100, Joe Orton wrote:
>> On Wed, Jul 23, 2025 at 09:08:48PM +0200, Daniel Sahlberg wrote:
>>> Hi,
>>>
>>> The patch below has been floating in dev@ for about a month. It looks looks
>>> like an improvement to me, but it is way out of my comfort zone.
>>>
>>> @Joe Orton <jo...@redhat.com> are you able to test this?
>>
>> Sure thing, I've fired off a test build against Fedora Rawhide with
>> Yasuhito's patch applied, will let you know how it goes.
>
> It failed as below. I'm testing 1.14.5 with the attached patch, which
> brings repository.py to match trunk + Yasuhito's patch.
>
> ======================================================================
> FAIL: test_replay_batons_refcounts (repository.SubversionRepositoryTestCase.test_replay_batons_refcounts)
> Issue SVN-4917: check ref-count of batons created and used in callbacks
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/builddir/build/BUILD/subversion-1.14.5-build/subversion-1.14.5/subversion/bindings/swig/python/tests/repository.py", line 447, in test_replay_batons_refcounts
> self.assertEqual(sys.getrefcount(baton_tuple[2]), baton_tuple[3],
> ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "leak on baton %s after replay without errors"
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> % repr(baton_tuple))
> ^^^^^^^^^^^^^^^^^^^^
> AssertionError: 2 != 0 : leak on baton (b'dir baton', b'', (b'', b'', 11), 0) after replay without errors
> ----------------------------------------------------------------------
Thank you for testing.
The intention of those tests are detection of extra reference
remaining after calling callback functions. However, with deferred
reference counting and biased reference counting, it cannot work
as I(we?) expected. So I don't think the changes on r1926575 is
correct if no one cannot explain why this part of the test could
pass without modifying expected refcount.
I propose:
* Use weakref for the tests of this purpose
* Until those test is rewrited with using weakref, simply skip
them on Python >= 3.14.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Jul 24, 2025 at 11:41:20AM +0100, Joe Orton wrote:
> On Wed, Jul 23, 2025 at 09:08:48PM +0200, Daniel Sahlberg wrote:
> > Hi,
> >
> > The patch below has been floating in dev@ for about a month. It looks looks
> > like an improvement to me, but it is way out of my comfort zone.
> >
> > @Joe Orton <jo...@redhat.com> are you able to test this?
>
> Sure thing, I've fired off a test build against Fedora Rawhide with
> Yasuhito's patch applied, will let you know how it goes.
It failed as below. I'm testing 1.14.5 with the attached patch, which
brings repository.py to match trunk + Yasuhito's patch.
======================================================================
FAIL: test_replay_batons_refcounts (repository.SubversionRepositoryTestCase.test_replay_batons_refcounts)
Issue SVN-4917: check ref-count of batons created and used in callbacks
----------------------------------------------------------------------
Traceback (most recent call last):
File "/builddir/build/BUILD/subversion-1.14.5-build/subversion-1.14.5/subversion/bindings/swig/python/tests/repository.py", line 447, in test_replay_batons_refcounts
self.assertEqual(sys.getrefcount(baton_tuple[2]), baton_tuple[3],
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"leak on baton %s after replay without errors"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
% repr(baton_tuple))
^^^^^^^^^^^^^^^^^^^^
AssertionError: 2 != 0 : leak on baton (b'dir baton', b'', (b'', b'', 11), 0) after replay without errors
----------------------------------------------------------------------
Re: svn commit: r1926575 - in /subversion/trunk/subversion/bindings/swig/python/tests: mergeinfo.py repository.py
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jul 23, 2025 at 09:08:48PM +0200, Daniel Sahlberg wrote:
> Hi,
>
> The patch below has been floating in dev@ for about a month. It looks looks
> like an improvement to me, but it is way out of my comfort zone.
>
> @Joe Orton <jo...@redhat.com> are you able to test this?
Sure thing, I've fired off a test build against Fedora Rawhide with
Yasuhito's patch applied, will let you know how it goes.