Opened 5 years ago

Last modified 7 months ago

#17670 needs_work enhancement

Proper implementation of object pools

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.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) Commit: d7d56b13f702b6db60b8a361111aba24c2334bda
Dependencies: #24111 Stopgaps:

Description (last modified by 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.

Change History (38)

comment:1 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Get rid of hook_tp_functions() hack to Proper implementation of object pools

comment:3 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 3 years ago by jdemeyer

  • Cc roed saraedum caruso added
  • Milestone changed from sage-6.5 to sage-8.1

comment:5 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/proper_implementation_of_object_pools

comment:6 follow-ups: Changed 2 years ago by caruso

  • Commit set to 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 2 years ago by caruso (previous) (diff)

comment:7 in reply to: ↑ 6 Changed 2 years ago by jdemeyer

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 2 years ago by jdemeyer

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 2 years ago by jdemeyer

  • Dependencies set to #24111

comment:10 Changed 2 years ago by jdemeyer

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

  • Commit changed from 0d7616d52db1c30353cd68300b5f87e04b859cc8 to 997efe8966817bbb7e7eed83be03e6837a9636cd

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

  • Commit changed from 997efe8966817bbb7e7eed83be03e6837a9636cd to 2f5f4630685f6f71934cc8af8fc47db4c6df5d35

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 2 years ago by jdemeyer

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

  • Commit changed from 2f5f4630685f6f71934cc8af8fc47db4c6df5d35 to fe75dd551d3079b6bb2118af7864428760b82bea

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

fe75dd5Proper implementation of object pools

comment:15 Changed 2 years ago by git

  • Commit changed from fe75dd551d3079b6bb2118af7864428760b82bea to 41dd98b45d539c607fe2ae2a28f45de4d4b42195

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

41dd98bProper implementation of object pools

comment:16 Changed 2 years ago by git

  • Commit changed from 41dd98b45d539c607fe2ae2a28f45de4d4b42195 to a6b71b8bb8b9e341d641f4b54aeb07f8b911992e

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

a6b71b8Proper implementation of object pools

comment:17 Changed 2 years ago by git

  • Commit changed from a6b71b8bb8b9e341d641f4b54aeb07f8b911992e to 570901ed2ef1e05e90a255188cea8cbe8687013f

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 2 years ago by jdemeyer

  • Status changed from new to needs_review

comment:19 Changed 2 years ago by git

  • Commit changed from 570901ed2ef1e05e90a255188cea8cbe8687013f to 8d2ab64f1279af8d9e51dc91b7556aaecf4455b5

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

8d2ab64Proper implementation of object pools

comment:20 Changed 2 years ago by roed

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

  • Commit changed from 8d2ab64f1279af8d9e51dc91b7556aaecf4455b5 to c91279cfb6846692d66561624713f06468e5f251

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

c91279cProper implementation of object pools

comment:22 Changed 2 years ago by jdemeyer

  • 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 2 years ago by jdemeyer

  • Dependencies changed from #24111 to #24111, #24259

comment:24 Changed 2 years ago by caruso

  • Branch changed from u/jdemeyer/proper_implementation_of_object_pools to u/caruso/proper_implementation_of_object_pools

comment:25 Changed 2 years ago by jdemeyer

  • 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:

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 2 years ago by caruso

  • Branch changed from u/caruso/proper_implementation_of_object_pools to u/jdemeyer/proper_implementation_of_object_pools
  • Commit changed from e850b42f35a1f898d1f74738b1ec4ced99140586 to c91279cfb6846692d66561624713f06468e5f251

New commits:

c0b3a81Add Cython late include patch
c91279cProper implementation of object pools

comment:27 Changed 2 years ago by caruso

  • Branch changed from u/jdemeyer/proper_implementation_of_object_pools to u/caruso/proper_implementation_of_object_pools

comment:28 Changed 2 years ago by jdemeyer

  • Commit changed from c91279cfb6846692d66561624713f06468e5f251 to 0c8a03b4ac412407774379b2f8323e8e70a7c93e

???


New commits:

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

comment:29 Changed 2 years ago by caruso

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

comment:30 Changed 2 years ago by jdemeyer

So the branch should be reset to my branch then?

comment:31 Changed 2 years ago by caruso

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

Last edited 2 years ago by caruso (previous) (diff)

comment:32 Changed 2 years ago by jdemeyer

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

comment:33 Changed 2 years ago by jdemeyer

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

comment:34 Changed 21 months ago by roed

  • Keywords padicIMA added

comment:35 Changed 7 months ago by roed

  • 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 7 months ago by roed

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 7 months ago by roed

  • Branch changed from u/caruso/proper_implementation_of_object_pools to u/roed/proper_implementation_of_object_pools

comment:38 Changed 7 months ago by roed

  • 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:

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.