Opened 8 years ago

Last modified 3 years ago

#17670 needs_work enhancement

Proper implementation of object pools

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-8.1
Component: cython Keywords: padicIMA, padicBordeaux
Cc: David Roe, Julian Rüth, Xavier 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:

Status badges

Description (last modified by Jeroen Demeyer)

integer.pyx and real_double.pyx use hook_tp_functions() to change tp_new and tp_dealloc at run-time. 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 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)
Summary: Get rid of hook_tp_functions() hackProper implementation of object pools

comment:3 Changed 5 years ago by Jeroen Demeyer

Description: modified (diff)

comment:4 Changed 5 years ago by Jeroen Demeyer

Cc: David Roe Julian Rüth Xavier Caruso added
Milestone: sage-6.5sage-8.1

comment:5 Changed 5 years ago by Jeroen Demeyer

Branch: u/jdemeyer/proper_implementation_of_object_pools

comment:6 Changed 5 years ago by Xavier Caruso

Commit: 0d7616d52db1c30353cd68300b5f87e04b859cc8

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 p-adics 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:

0d7616dProper implementation of object pools
Last edited 5 years ago by Xavier Caruso (previous) (diff)

comment:7 in reply to:  6 Changed 5 years ago by Jeroen Demeyer

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 5 years ago by Jeroen Demeyer

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 5 years ago by Jeroen Demeyer

Dependencies: #24111

comment:10 Changed 5 years ago by Jeroen Demeyer

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 5 years ago by git

Commit: 0d7616d52db1c30353cd68300b5f87e04b859cc8997efe8966817bbb7e7eed83be03e6837a9636cd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

234b7c8Upgrade to Cython 0.27.2
64854cbAdd Cython late include patch
997efe8Proper implementation of object pools

comment:12 Changed 5 years ago by git

Commit: 997efe8966817bbb7e7eed83be03e6837a9636cd2f5f4630685f6f71934cc8af8fc47db4c6df5d35

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2f5f463Proper implementation of object pools

comment:13 in reply to:  6 Changed 5 years ago by Jeroen Demeyer

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 5 years ago by git

Commit: 2f5f4630685f6f71934cc8af8fc47db4c6df5d35fe75dd551d3079b6bb2118af7864428760b82bea

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fe75dd5Proper implementation of object pools

comment:15 Changed 5 years ago by git

Commit: fe75dd551d3079b6bb2118af7864428760b82bea41dd98b45d539c607fe2ae2a28f45de4d4b42195

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

41dd98bProper implementation of object pools

comment:16 Changed 5 years ago by git

Commit: 41dd98b45d539c607fe2ae2a28f45de4d4b42195a6b71b8bb8b9e341d641f4b54aeb07f8b911992e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a6b71b8Proper implementation of object pools

comment:17 Changed 5 years ago by git

Commit: a6b71b8bb8b9e341d641f4b54aeb07f8b911992e570901ed2ef1e05e90a255188cea8cbe8687013f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c331d9bGenerate eclib includes in a predictable order
c0b3a81Add Cython late include patch
570901eProper implementation of object pools

comment:18 Changed 5 years ago by Jeroen Demeyer

Status: newneeds_review

comment:19 Changed 5 years ago by git

Commit: 570901ed2ef1e05e90a255188cea8cbe8687013f8d2ab64f1279af8d9e51dc91b7556aaecf4455b5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8d2ab64Proper implementation of object pools

comment:20 Changed 5 years ago by David Roe

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 5 years ago by git

Commit: 8d2ab64f1279af8d9e51dc91b7556aaecf4455b5c91279cfb6846692d66561624713f06468e5f251

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c91279cProper implementation of object pools

comment:22 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

There is worrying doctest failure on the patchbot that I'll need to check.

comment:23 Changed 5 years ago by Jeroen Demeyer

Dependencies: #24111#24111, #24259

comment:24 Changed 5 years ago by Xavier Caruso

Branch: u/jdemeyer/proper_implementation_of_object_poolsu/caruso/proper_implementation_of_object_pools

comment:25 Changed 5 years ago by Jeroen Demeyer

Commit: c91279cfb6846692d66561624713f06468e5f251e850b42f35a1f898d1f74738b1ec4ced99140586
Dependencies: #24111, #24259#24111

I do intend to come back to this ticket, but this really needs a new version of Cython.


Last 10 new commits:

c3c8692Doctest for the class pRational
9780da5More doctests
b9b4a34Small fix in list_of_padics
241bd7aMerge remote-tracking branch 'trac/develop' into t/23505/lattice_precision
2a94835Merge branch 'u/saraedum/lattice_precision' of git://trac.sagemath.org/sage into lattice_precision
23241e2Address some of Julian's comments
2c334e7100% doctest coverage (hopefully)
a67c96citeritems, python 3 compatibility
6593e23Merge branch 'develop' into lattice_precision
e850b42Merge branch 'u/jdemeyer/proper_implementation_of_object_pools' of git://trac.sagemath.org/sage into proper_pool

comment:26 Changed 5 years ago by Xavier Caruso

Branch: u/caruso/proper_implementation_of_object_poolsu/jdemeyer/proper_implementation_of_object_pools
Commit: e850b42f35a1f898d1f74738b1ec4ced99140586c91279cfb6846692d66561624713f06468e5f251

New commits:

c0b3a81Add Cython late include patch
c91279cProper implementation of object pools

comment:27 Changed 5 years ago by Xavier Caruso

Branch: u/jdemeyer/proper_implementation_of_object_poolsu/caruso/proper_implementation_of_object_pools

comment:28 Changed 5 years ago by Jeroen Demeyer

Commit: c91279cfb6846692d66561624713f06468e5f2510c8a03b4ac412407774379b2f8323e8e70a7c93e

???


New commits:

0c8a03bMerge remote-tracking branch 'trac/develop' into t/17670/proper_implementation_of_object_pools

comment:29 Changed 5 years ago by Xavier Caruso

Sorry, I merged by mistake ticket #23505 into this one. This should be fixed now.

comment:30 Changed 5 years ago by Jeroen Demeyer

So the branch should be reset to my branch then?

comment:31 Changed 5 years ago by Xavier Caruso

Well, I also resolved a couple of conflicts, so that my branch is supposed to merge properly with develop.

Last edited 5 years ago by Xavier Caruso (previous) (diff)

comment:32 Changed 5 years ago by Jeroen Demeyer

I see. But given that #24111 is not a "real" ticket, it's better to rebase instead of merge.

comment:33 Changed 5 years ago by Jeroen Demeyer

#24111 just became a real ticket and it needs review :-)

comment:34 Changed 4 years ago by David Roe

Keywords: padicIMA added

comment:35 Changed 3 years ago by David Roe

Keywords: padicBordeaux added

Xavier, Julian and I are going to be working on p-adics this coming week in Bordeaux. I'm just tagging this ticket as one we might want to look at.

comment:36 Changed 3 years ago by David Roe

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 3 years ago by David Roe

Branch: u/caruso/proper_implementation_of_object_poolsu/roed/proper_implementation_of_object_pools

comment:38 Changed 3 years ago by David Roe

Commit: 0c8a03b4ac412407774379b2f8323e8e70a7c93ed7d56b13f702b6db60b8a361111aba24c2334bda

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:

d7d56b1Merge branch 'u/caruso/proper_implementation_of_object_pools' of git://trac.sagemath.org/sage into t17670_object_pools
Note: See TracTickets for help on using tickets.