Opened 7 years ago
Closed 7 years ago
#15506 closed defect (fixed)
Fix another "recursion depth exceeded" in memory deallocation for weak dictionaries
Reported by: | nthiery | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.1 |
Component: | performance | Keywords: | |
Cc: | sage-combinat, vbraun, jdemeyer, SimonKing | Merged in: | |
Authors: | Nils Bruin | Reviewers: | Simon King |
Report Upstream: | N/A | Work issues: | |
Branch: | u/SimonKing/ticket/15506 (Commits, GitHub, GitLab) | Commit: | 614777716fa30b68fd22322435cc273430209f7d |
Dependencies: | #15432 | Stopgaps: |
Description
See: http://trac.sagemath.org/ticket/10963?replyto=174#comment:159 and follow up for details
This is a follow up to #13394 #15069, and a blocker for #10963.
Change History (28)
comment:1 Changed 7 years ago by
- Branch set to u/nbruin/ticket/15506
- Created changed from 12/10/13 06:27:39 to 12/10/13 06:27:39
- Modified changed from 12/10/13 06:27:39 to 12/10/13 06:27:39
comment:2 Changed 7 years ago by
- Commit set to 5de9c7c33caab20a44a8ba955d3b12548f2f0c25
comment:3 Changed 7 years ago by
- Commit changed from 5de9c7c33caab20a44a8ba955d3b12548f2f0c25 to e23ec990e914edea0cff655ff188716279b33afc
Branch pushed to git repo; I updated commit sha1. New commits:
e23ec99 | Merge branch 'ticket/15432' into ticket/15506 |
4ac6866 | trac #15432: Code context manager in iterator via try/finally. This saves an entire class and some reference management.Z |
8b70d1d | trac #15432: also equip iteration context with a weak reference |
9bb9241 | trac #15432: use a callback class object holding a weak reference for sage.misc.weak_dict.WeakValueDictionary? |
comment:4 Changed 7 years ago by
comment:5 follow-up: ↓ 6 Changed 7 years ago by
That was quick :-) Thank you Nils!
Is there a reason for not putting the above code that verifies the solution as a doctest?
Other than that, what shall we do? Backport this ticket and its dependencies to mercurial, or move #10963 to git?
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 7 years ago by
Replying to nthiery:
Is there a reason for not putting the above code that verifies the solution as a doctest?
No, if you want to add a doctest, go ahead.
Backport this ticket and its dependencies to mercurial, or move #10963 to git?
I have no preference. Given that you'd have to backport #15367 as well, I think you'd better bite the bullet and go with git.
comment:7 in reply to: ↑ 6 Changed 7 years ago by
Replying to nbruin:
Replying to nthiery:
Is there a reason for not putting the above code that verifies the solution as a doctest?
No, if you want to add a doctest, go ahead.
Backport this ticket and its dependencies to mercurial, or move #10963 to git?
I have no preference. Given that you'd have to backport #15367 as well, I think you'd better bite the bullet and go with git.
I am fine with both, as long as it goes quickly. Volker, Jeroen, what do you think from a release management point of view?
comment:8 Changed 7 years ago by
- Milestone changed from sage-5.13 to sage-6.0
- Priority changed from blocker to major
I don't see a good reason to force this ticket in Sage 5.13. So it should not be a blocker and should be postponed to Sage 6.0 which means git.
comment:9 Changed 7 years ago by
- Cc vbraun jdemeyer SimonKing added
comment:10 Changed 7 years ago by
- Status changed from new to needs_review
comment:11 Changed 7 years ago by
- Milestone changed from sage-6.0 to sage-6.1
comment:12 follow-up: ↓ 13 Changed 7 years ago by
can we get this reviewed? ;-)
comment:13 in reply to: ↑ 12 Changed 7 years ago by
Replying to vbraun:
can we get this reviewed? ;-)
I suppose this is independent of the patch:
File "src/sage/dev/sagedev.py", line 89, in sage.dev.sagedev.SageDev Failed example: dev._sagedev Exception raised: Traceback (most recent call last): File "/tmp/tmpo1ll9a-sage-git-temp-15506/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run self.execute(example, compiled, test.globs) File "/tmp/tmpo1ll9a-sage-git-temp-15506/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute exec compiled in globs File "<doctest sage.dev.sagedev.SageDev[0]>", line 1, in <module> dev._sagedev File "lazy_import.pyx", line 312, in sage.misc.lazy_import.LazyImport.__getattr__ (sage/misc/lazy_import.c:2402) File "lazy_import.pyx", line 248, in sage.misc.lazy_import.LazyImport._get_object (sage/misc/lazy_import.c:1885) File "/tmp/tmpo1ll9a-sage-git-temp-15506/local/lib/python2.7/site-packages/sage/dev/sagedev_wrapper.py", line 250, in <module> dev = SageDevWrapper(SageDev()) File "/tmp/tmpo1ll9a-sage-git-temp-15506/local/lib/python2.7/site-packages/sage/dev/sagedev.py", line 171, in __init__ self.__branch_to_ticket = SavingDict(ticket_file) File "/tmp/tmpo1ll9a-sage-git-temp-15506/local/lib/python2.7/site-packages/sage/dev/saving_dict.py", line 116, in __init__ self._write() File "/tmp/tmpo1ll9a-sage-git-temp-15506/local/lib/python2.7/site-packages/sage/dev/saving_dict.py", line 236, in _write assert os.path.abspath(self._filename).startswith(SAGE_TMP), error AssertionError: write attempt to a saving_dict in a doctest
comment:14 Changed 7 years ago by
Is it really needed to additionally import this during startup:
+New: + sage.combinat.binary_recurrence_sequences + sage.libs.pari.handle_error + sage.misc.weak_dict
I am particularly talking about binary recurrence sequences.
comment:15 Changed 7 years ago by
Has this really been introduced by this branch:
0015-Trac-14320-More-doctests-from-the-book-Calcul-math-m.patch:37 + sage: R100 = RealField(100) # prÃ©cision : 100 bits.$ 0015-Trac-14320-More-doctests-from-the-book-Calcul-math-m.patch:43 + sage: Rdefaut = RealField() # prÃ©cision par dÃ©faut de 53 bits$ 0015-Trac-14320-More-doctests-from-the-book-Calcul-math-m.patch:64 + sage: x = 1.0 # x appartient Ã RealField()$ 0015-Trac-14320-More-doctests-from-the-book-Calcul-math-m.patch:65 + sage: x = 0.1e+1 # idem : x appartient Ã RealField()$ 0015-Trac-14320-More-doctests-from-the-book-Calcul-math-m.patch:67 + sage: x = RDF(1) # x est un flottant double prÃ©cision machine$ 0015-Trac-14320-More-doctests-from-the-book-Calcul-math-m.patch:68 + sage: x = RDF(1.) # idem : x est un flottant double prÃ©cision$ 0015-Trac-14320-More-doctests-from-the-book-Calcul-math-m.patch:72 + sage: x = R(1) # x est un flottant de prÃ©cision 20 bits$
Or is this from #14320?
comment:16 follow-up: ↓ 17 Changed 7 years ago by
Concerning additional imports and Calcul-math things:
None of that is directly effected by commits on this ticket, so it has to from the state of "master" on which this branch is based.
This ticket is only meant to change weak_dict.pyx
and does not introduce any import statements there. Feel free to use the commits here to build a new branch based on a different tree if there are things in the way (note that #15432 has been merged in the branch here; without it there's only one commit here)
comment:17 in reply to: ↑ 16 ; follow-up: ↓ 19 Changed 7 years ago by
Replying to nbruin:
Concerning additional imports and Calcul-math things:
None of that is directly effected by commits on this ticket, so it has to from the state of "master" on which this branch is based.
OK, so, it is a bug in the plugins.
This ticket is only meant to change
weak_dict.pyx
and does not introduce any import statements there. Feel free to use the commits here to build a new branch based on a different tree if there are things in the way (note that #15432 has been merged in the branch here; without it there's only one commit here)
And now I am really confused.
In #15432, you introduce a weak reference from the callback to the weak value dictionary. Namely, on #15432, you point out that when passing the weak value dictionary to the callback via a closure then there are examples requiring many garbage collections to clear a weak value dictionary.
Now, you revert this change. Can you explain why you did so? Doesn't this re-introduce the problem that you fixed in #15432? If not: Why not? And why did you merge #15432 into this branch? What is left from #15432 in the HEAD of this branch?
comment:18 Changed 7 years ago by
Hm. I just tested that the problematic behaviour (namely: Numerous garbage collection cycles required to clear the dict) does not occur with your branch. Could it perhaps be the case that I was (again) reading in a wrong way the diff shown by trac?
comment:19 in reply to: ↑ 17 Changed 7 years ago by
Replying to SimonKing:
And why did you merge #15432 into this branch?
I figured that this was the new way to express a "dependency" on another ticket. It results in the branch I would have ended up with if I had started this ticket based on the branch there.
What is left from #15432 in the HEAD of this branch?
Everything, I think. I really just merged and it did so without conflict. Certainly all the commits listed at #15432 show up in the history here.
comment:20 follow-up: ↓ 21 Changed 7 years ago by
Then perhaps I was misreading what trac showed me as diff.
I now took #15432, merged with master, and compare it with the branch from here, merged with master.
These are the differences I see with git diff ticket/15432 ticket/15506
:
- You put certain to-be-deleted things into tuples, in order to use Python's trashcan.
- You remove the
_IterationContext
and replace it by cdef methods. So, instead of a context, you havetry: self._enter_iter() ... finally: self._exit_iter()
. I suppose that's for speed.
So, are these the changes I have to review here?
Best regards, Simon
comment:21 in reply to: ↑ 20 Changed 7 years ago by
Replying to SimonKing:
These are the differences I see with
git diff ticket/15432 ticket/15506
:
- You put certain to-be-deleted things into tuples, in order to use Python's trashcan.
Correct (isn't it a list rather than a tuple? Doesn't matter for our purposes, though).
- You remove the
_IterationContext
and replace it by cdef methods. So, instead of a context, you havetry: self._enter_iter() ... finally: self._exit_iter()
. I suppose that's for speed.
That's from #15432. It's more than speed: it also saves us from having to keep a weakened reference in the IterationContext?, so it makes it easier to avoid circular references. I was actually surprised to see the complication that a context produced here.
In addition to these changes, #15432 also introduces an WeakKeyDictionary? eraser class to make it possible to keep a weak reference (and avoid circular references).
I'd recommend just reviewing all of this here and be done with it (unless you feel any of this is controversial of course)
comment:22 Changed 7 years ago by
- Dependencies set to #15432
Simon, are you still reviewing this and #15432? The list of commits in question is http://git.sagemath.org/sage.git/log/?h=u%2Fnbruin%2Fticket%2F15506&qt=range&q=develop..u%2Fnbruin%2Fticket%2F15506
comment:23 Changed 7 years ago by
comment:24 Changed 7 years ago by
Am I right that I don't need to push a merge with my review-commit from #15432, since there is no merge conflict?
comment:25 Changed 7 years ago by
Yes, if there is no conflict then you don't have to merge.
comment:26 Changed 7 years ago by
- Branch changed from u/nbruin/ticket/15506 to u/SimonKing/ticket/15506
- Commit changed from e23ec990e914edea0cff655ff188716279b33afc to 614777716fa30b68fd22322435cc273430209f7d
- Reviewers set to Simon King
- Status changed from needs_review to positive_review
I have added the example from commit:2 as a doc test (otherwise the fix wouldn't be tested against).
All tests pass (including the new one), and the code makes sense. I only wonder if we can use Python's trashcan in a different more efficient way (such as to avoid the creation of a list of length two during deletion of a dictionary item). But I think this ticket is about fixing a bug in the first place, and if we see the need for yet more efficiency, we can open another ticket.
I.e.: Positive review.
comment:27 Changed 7 years ago by
Yeah, that's a nice Chrismas present :-) Thanks Simon, thanks Nils!
comment:28 Changed 7 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
This should do the trick for
WeakValueDictionary
. Merge #15367 to get the same benefit forMonoDict
andTripleDict
.Code that verifies the problem has been solved:
This used to fail, but with the code on this branch it works.
New commits: