Proper implementation of object pools

[jdemeyer]

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.

Depends on #24111

CC: @roed314 @saraedum @xcaruso

Component: cython

Keywords: padicIMA, padicBordeaux

Author: Jeroen Demeyer

Branch/Commit: u/roed/proper_implementation_of_object_pools @ d7d56b1

Issue created by migration from https://trac.sagemath.org/ticket/17670

[jdemeyer]

Branch: u/jdemeyer/proper_implementation_of_object_pools

[xcaruso]

Commit: 0d7616d

[xcaruso]

comment:6

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:

0d7616d

Proper implementation of object pools

[jdemeyer]

comment:7

Replying to @xcaruso:

Why are you limiting the number of pools to 4? Isn't it too small?

4 per Cython module.

[jdemeyer]

comment:8

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.

[jdemeyer]

Dependencies: #24111

[jdemeyer]

comment:10

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.

[sagetrac-git]

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

234b7c8	Upgrade to Cython 0.27.2
64854cb	Add Cython late include patch
997efe8	Proper implementation of object pools

[sagetrac-git]

Changed commit from 0d7616d to 997efe8

[sagetrac-git]

Changed commit from 997efe8 to 2f5f463

[sagetrac-git]

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

2f5f463

Proper implementation of object pools

[jdemeyer]

comment:13

Replying to @xcaruso:

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"?

[sagetrac-git]

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

fe75dd5

Proper implementation of object pools

[sagetrac-git]

Changed commit from 2f5f463 to fe75dd5

[sagetrac-git]

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

41dd98b

Proper implementation of object pools

[sagetrac-git]

Changed commit from fe75dd5 to 41dd98b

[sagetrac-git]

Changed commit from 41dd98b to a6b71b8

[sagetrac-git]

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

a6b71b8

Proper implementation of object pools

[sagetrac-git]

Changed commit from a6b71b8 to 570901e

[sagetrac-git]

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

c331d9b	Generate eclib includes in a predictable order
c0b3a81	Add Cython late include patch
570901e	Proper implementation of object pools

[sagetrac-git]

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

c91279c

Proper implementation of object pools

[sagetrac-git]

Changed commit from 8d2ab64 to c91279c

[jdemeyer]

comment:22

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

[jdemeyer]

Changed dependencies from #24111 to #24111, #24259

[xcaruso]

Changed branch from u/jdemeyer/proper_implementation_of_object_pools to u/caruso/proper_implementation_of_object_pools

[jdemeyer]

comment:25

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 remote-tracking 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

[jdemeyer]

Changed dependencies from #24111, #24259 to #24111

[jdemeyer]

Changed commit from c91279c to e850b42

[xcaruso]

Changed branch from u/caruso/proper_implementation_of_object_pools to u/jdemeyer/proper_implementation_of_object_pools

[xcaruso]

New commits:

c0b3a81	Add Cython late include patch
c91279c	Proper implementation of object pools

[xcaruso]

Changed commit from e850b42 to c91279c

[xcaruso]

Changed branch from u/jdemeyer/proper_implementation_of_object_pools to u/caruso/proper_implementation_of_object_pools

[jdemeyer]

comment:28

???

---

New commits:

0c8a03b

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

[jdemeyer]

Changed commit from c91279c to 0c8a03b

[xcaruso]

comment:29

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

[jdemeyer]

comment:30

So the branch should be reset to my branch then?

[xcaruso]

comment:31

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

[jdemeyer]

comment:32

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

[jdemeyer]

comment:33

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

[roed314]

Changed keywords from none to padicIMA

[roed314]

comment:35

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.

[roed314]

Changed keywords from padicIMA to padicIMA, padicBordeaux

[roed314]

comment:36

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?

[roed314]

Changed branch from u/caruso/proper_implementation_of_object_pools to u/roed/proper_implementation_of_object_pools

[roed314]

comment:38

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

[roed314]

Changed commit from 0c8a03b to d7d56b1