Opened 7 years ago
Last modified 2 years ago
#17670 needs_work enhancement
Proper implementation of object pools
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  cython  Keywords:  padicIMA, padicBordeaux 
Cc:  roed, saraedum, caruso  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/roed/proper_implementation_of_object_pools (Commits, GitHub, GitLab)  Commit:  d7d56b13f702b6db60b8a361111aba24c2334bda 
Dependencies:  #24111  Stopgaps: 
Description (last modified by )
integer.pyx
and real_double.pyx
use hook_tp_functions()
to change tp_new
and tp_dealloc
at runtime. It seems that one cannot avoid such hacks with the current Cython, but at least we could provide a cleaner interface.
Change History (38)
comment:1 Changed 7 years ago by
 Description modified (diff)
comment:2 Changed 7 years ago by
 Description modified (diff)
 Summary changed from Get rid of hook_tp_functions() hack to Proper implementation of object pools
comment:3 Changed 4 years ago by
 Description modified (diff)
comment:4 Changed 4 years ago by
 Cc roed saraedum caruso added
 Milestone changed from sage6.5 to sage8.1
comment:5 Changed 4 years ago by
 Branch set to u/jdemeyer/proper_implementation_of_object_pools
comment:6 followups: ↓ 7 ↓ 13 Changed 4 years ago by
 Commit set to 0d7616d52db1c30353cd68300b5f87e04b859cc8
comment:7 in reply to: ↑ 6 Changed 4 years ago by
Replying to caruso:
Why are you limiting the number of pools to 4? Isn't it too small?
4 per Cython module.
comment:8 Changed 4 years ago by
A more general comment about pools is that everybody has a different set of features. For example, the Integer
pool that we currently have in Sage is actually more than just a pool. It also constructs new objects quickly from a prototype object.
comment:9 Changed 4 years ago by
 Dependencies set to #24111
comment:10 Changed 4 years ago by
Let me continue with this... I'm adding a dependency on #24111 because that makes it a lot easier to write modules which are partially implemented in C and partially in Cython.
comment:11 Changed 4 years ago by
 Commit changed from 0d7616d52db1c30353cd68300b5f87e04b859cc8 to 997efe8966817bbb7e7eed83be03e6837a9636cd
comment:12 Changed 4 years ago by
 Commit changed from 997efe8966817bbb7e7eed83be03e6837a9636cd to 2f5f4630685f6f71934cc8af8fc47db4c6df5d35
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2f5f463  Proper implementation of object pools

comment:13 in reply to: ↑ 6 Changed 4 years ago by
Replying to caruso:
I saw that you stored the address of the pool in a dictionary. Did you try to estimate the time penalty of this dictionary lookup?
I am not doing any dictionary lookup (except for putting the pool in the dict). I am storing the pool in the dict for 2 reasons: for debugging purposes and to ensure that some reference is kept to the pool (to prevent it from being deleted). I do not use this dict in the implementation.
By design, your implementation cannot allow what I've called local pools (several different pools are possible for a same type) and global pools (a unique pool is shared all by all elements having a given type). I'm a bit sad about this because I think that having different pools may really make sense
I never understood the need for local pools. I thought that you stored the pool in the parent only for practical purposes.
and then objects with different sizes
What do you mean with "objects with different sizes"?
comment:14 Changed 4 years ago by
 Commit changed from 2f5f4630685f6f71934cc8af8fc47db4c6df5d35 to fe75dd551d3079b6bb2118af7864428760b82bea
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fe75dd5  Proper implementation of object pools

comment:15 Changed 4 years ago by
 Commit changed from fe75dd551d3079b6bb2118af7864428760b82bea to 41dd98b45d539c607fe2ae2a28f45de4d4b42195
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
41dd98b  Proper implementation of object pools

comment:16 Changed 4 years ago by
 Commit changed from 41dd98b45d539c607fe2ae2a28f45de4d4b42195 to a6b71b8bb8b9e341d641f4b54aeb07f8b911992e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a6b71b8  Proper implementation of object pools

comment:17 Changed 4 years ago by
 Commit changed from a6b71b8bb8b9e341d641f4b54aeb07f8b911992e to 570901ed2ef1e05e90a255188cea8cbe8687013f
comment:18 Changed 4 years ago by
 Status changed from new to needs_review
comment:19 Changed 4 years ago by
 Commit changed from 570901ed2ef1e05e90a255188cea8cbe8687013f to 8d2ab64f1279af8d9e51dc91b7556aaecf4455b5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8d2ab64  Proper implementation of object pools

comment:20 Changed 4 years ago by
Cool! I read through the code, but am too sleepy to make comments (or test further) tonight. Overall, it looks good and tests pass!
comment:21 Changed 4 years ago by
 Commit changed from 8d2ab64f1279af8d9e51dc91b7556aaecf4455b5 to c91279cfb6846692d66561624713f06468e5f251
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c91279c  Proper implementation of object pools

comment:22 Changed 4 years ago by
 Status changed from needs_review to needs_work
There is worrying doctest failure on the patchbot that I'll need to check.
comment:23 Changed 4 years ago by
 Dependencies changed from #24111 to #24111, #24259
comment:24 Changed 4 years ago by
 Branch changed from u/jdemeyer/proper_implementation_of_object_pools to u/caruso/proper_implementation_of_object_pools
comment:25 Changed 4 years ago by
 Commit changed from c91279cfb6846692d66561624713f06468e5f251 to e850b42f35a1f898d1f74738b1ec4ced99140586
 Dependencies changed from #24111, #24259 to #24111
I do intend to come back to this ticket, but this really needs a new version of Cython.
Last 10 new commits:
c3c8692  Doctest for the class pRational

9780da5  More doctests

b9b4a34  Small fix in list_of_padics

241bd7a  Merge remotetracking branch 'trac/develop' into t/23505/lattice_precision

2a94835  Merge branch 'u/saraedum/lattice_precision' of git://trac.sagemath.org/sage into lattice_precision

23241e2  Address some of Julian's comments

2c334e7  100% doctest coverage (hopefully)

a67c96c  iteritems, python 3 compatibility

6593e23  Merge branch 'develop' into lattice_precision

e850b42  Merge branch 'u/jdemeyer/proper_implementation_of_object_pools' of git://trac.sagemath.org/sage into proper_pool

comment:26 Changed 4 years ago by
 Branch changed from u/caruso/proper_implementation_of_object_pools to u/jdemeyer/proper_implementation_of_object_pools
 Commit changed from e850b42f35a1f898d1f74738b1ec4ced99140586 to c91279cfb6846692d66561624713f06468e5f251
comment:27 Changed 4 years ago by
 Branch changed from u/jdemeyer/proper_implementation_of_object_pools to u/caruso/proper_implementation_of_object_pools
comment:28 Changed 4 years ago by
 Commit changed from c91279cfb6846692d66561624713f06468e5f251 to 0c8a03b4ac412407774379b2f8323e8e70a7c93e
???
New commits:
0c8a03b  Merge remotetracking branch 'trac/develop' into t/17670/proper_implementation_of_object_pools

comment:29 Changed 4 years ago by
Sorry, I merged by mistake ticket #23505 into this one. This should be fixed now.
comment:30 Changed 4 years ago by
So the branch should be reset to my branch then?
comment:31 Changed 4 years ago by
Well, I also resolved a couple of conflicts, so that my branch is supposed to merge properly with develop.
comment:32 Changed 4 years ago by
I see. But given that #24111 is not a "real" ticket, it's better to rebase instead of merge.
comment:33 Changed 4 years ago by
#24111 just became a real ticket and it needs review :)
comment:34 Changed 3 years ago by
 Keywords padicIMA added
comment:35 Changed 2 years ago by
 Keywords padicBordeaux added
Xavier, Julian and I are going to be working on padics this coming week in Bordeaux. I'm just tagging this ticket as one we might want to look at.
comment:36 Changed 2 years ago by
Jeroen, we're happy to work on this a bit this week (in particular, resolving merge conflicts). Do you have any known issues with what you've written?
comment:37 Changed 2 years ago by
 Branch changed from u/caruso/proper_implementation_of_object_pools to u/roed/proper_implementation_of_object_pools
comment:38 Changed 2 years ago by
 Commit changed from 0c8a03b4ac412407774379b2f8323e8e70a7c93e to d7d56b13f702b6db60b8a361111aba24c2334bda
I resolved the merge conflicts, which were in the cython version level and integer.pyx
, but now Sage crashes on startup. When I ran make build
, it surprisingly only rebuilt 4 Cython files. Given that there are patches to Cython in this ticket, I think I might need to reinstall Cython with sage f cython
, but I think I'm going to work on other tickets for now rather than try to debug this.
New commits:
d7d56b1  Merge branch 'u/caruso/proper_implementation_of_object_pools' of git://trac.sagemath.org/sage into t17670_object_pools

I've just had a look at your implementation (sorry for the delay).
I saw that you stored the address of the pool in a dictionary. Did you try to estimate the time penalty of this dictionary lookup? For instance, did you compare timings between (1) your implementation and (2) the current implementation for integers?
By design, your implementation cannot allow what I've called local pools (several different pools are possible for a same type) and global pools (a unique pool is shared all by all elements having a given type). I'm a bit sad about this because I think that having different pools may really make sense (for instance for padics when we are dealing with many different primes p and then objects with different sizes). But OK, I can accept this :).
Why are you limiting the number of pools to 4? Isn't it too small?
New commits:
Proper implementation of object pools