Avoid mutate Integer.value at a few more places, remove sig_occurred() check in Integer fast_tp_dealloc

[user202729]

Follow-up to #40556. Now the sig_occurred() check in fast_tp_dealloc is removed.

---

This is the effect on the last commit, changing divisors() to use sig_check().

Before:

sage: n = prod(primes_first_n(17))
....: %timeit ignore = n.divisors()
20 ms ± 245 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: n = prod(primes_first_n(17))
....: %timeit ignore = n.divisors()
20.5 ms ± 616 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: n = prod(primes_first_n(17))
....: %timeit ignore = n.divisors()
20 ms ± 302 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)

After:

sage: n = prod(primes_first_n(17))
....: %timeit ignore = n.divisors()
20.6 ms ± 424 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: n = prod(primes_first_n(17))
....: %timeit ignore = n.divisors()
20.5 ms ± 254 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: n = prod(primes_first_n(17))
....: %timeit ignore = n.divisors()
20.7 ms ± 206 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

⌛ Dependencies

[github-actions]

Documentation preview for this PR (built with commit da6ef59; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[orlitzky]

This is probably not new, but the Ctrl-C test fails reliably for me:

sage: for i in range(10000):
....:     print(f"i: {i}")
....:     with ensure_interruptible_after(RDF.random_element(1e-3, 0.5)):
....:         _ = n.divisors()
....: 
i: 0
i: 1
i: 2
---------------------------------------------------------------------------
AlarmInterrupt                            Traceback (most recent call last)
...
RuntimeError: Function is not interruptible within 0.3484 seconds, only after 0.5503 seconds

[orlitzky]

The rest looks reasonable. There are some opportunities for simultaneous init/set (https://gmplib.org/manual/Simultaneous-Integer-Init-_0026-Assign) I think.

[user202729]

init/set would be strictly worse in this case, since Function: void mpz_init_set (mpz_t rop, const mpz_t op) would copy from op, while we want to move from op. So the init as empty (O(1)) → swap (O(1)) → delete the now-empty mpz (also O(1)).

too bad the init/swap/delete empty is (I suspect) not inlined, if it were inlined it would be almost O(0). (that's wrong use of big-O notation, I know.

[orlitzky]

Not at the end... I meant in situations like

-        cdef Integer n = Integer(self)  # need a copy as it is modified below
-
+        cdef mpz_t mm, n
+        mpz_init(mm)
+        mpz_init(n)
+        mpz_set(n, self.value)
+        mpz_set(mm, (<Integer>m).value)

Surely mpz_init_set has to be faster than mpz_init followed immediately by mpz_set?

[user202729]

okay, you're right.

[orlitzky]

Probably a topic for wider discussion, but it always seemed weird to me that we cache the first 100 integers to be deallocated. Seems like it would be a lot simpler to cache 0-10, the first few primes, the first few powers of those primes, etc. There are a lot of commonly used integers and 100 isn't very many. It would eliminate a lot of voodoo if we just picked 100 that everyone likes.

[user202729]

we cache the first 100 integers to be deallocated

no, the point is if you do

while True:
    <create a new Integer object>
    <destroy the just-created Integer object>

then you don't incur the cost of allocation/deallocation every loop iteration.

[user202729]

the Ctrl-C test fails reliably for me

seems to work on my machine (though I'm still on 10.8.beta5), so not much I can do.

I guess if it makes the test work better you can set it to 0.5s or something… 0.5s is still acceptable to human observer right.

[user202729]

okay I can reproduce it once in roughly 50 tries.

worth investigating, I suppose.

[orlitzky]

we cache the first 100 integers to be deallocated

no, the point is if you do

while True:
    <create a new Integer object>
    <destroy the just-created Integer object>

then you don't incur the cost of allocation/deallocation every loop iteration.

I know what it's trying to do, but 100 is not very many. If that loop is the first thing you do in a sage session, you're probably OK; but if you mess around for a while first, the pool is going to be filled with integers you don't care about. Then when you write,

for k in range(10):
     p = ZZ(2^k)
     ...

the extremely common powers of two will still have to be allocated and deallocated each time because the pool is filled with random numbers.

[user202729]

after adding the latest round of sig_check:

• interrupt error is an order of magnitude rarer
• performance is negligibly affected

sage: n = prod(primes_first_n(17))
sage: %timeit ignore = n.divisors()
20.8 ms ± 521 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit ignore = n.divisors()
20.5 ms ± 221 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit ignore = n.divisors()
20.7 ms ± 488 μs per loop (mean ± std. dev. of 7 runs, 10 loops each)

• for one case of interrupt error, I checked and the interrupt fall right in the middle of a PyObject_Malloc. Understandably that can be slow, and we really shouldn't interrupt that otherwise Python heap may be in an inconsistent state. so I just increase the tolerance.

[user202729]

if you mess around for a while first, the pool is going to be filled with integers you don't care about

sorry, I don't understand. See the comments below

# pool size: 0
a = []
for i in range(100):
	a.append(ZZ.random_element())  # 100 allocations incurred here
# pool size: 0
a = []
# pool size: 100
for i in range(100):
	a.append(ZZ(2**i))  # 0 allocations (for the Python objects) incurred here
	                    # **because** the pool is full of random stuff, not **despite** it
						# (the newly-returned objects by `ZZ(2**i)` is taken **from** the pool,
						# then overwritten with the ZZ(2**i) value)
# pool size: 0

[user202729]

by the way, we do have a separate pool for small integers, but in order to create such an integer you need to explicitly invoke smallInteger.

doing a mpz_fits_slong_p + mpz_get_si takes O(1), but allocating a new Python object takes O(1) too. It's not a priori clear whether aggressively check to use the pool is profitable. (currently functions that return values that is likely small explicitly call smallInteger)

[orlitzky]

Ok, I concede, I will have to instrument integer.pyx with some helper functions and then come back if I still think there's room for improvement.

[orlitzky]

The tests are better now, I'm at 15,000 iterations and counting. This probably needs a rebase onto develop (git am didn't work, I had to patch -p1 with fuzz). But otherwise LGTM now.

[user202729]

Huh, I didn't notice there's merge conflict.

If you already have a version merged into develop, maybe you can send it somewhere (e.g. push it to your fork at some branch)? Thanks. Actually GitHub can merge it cleanly, I wonder why...

[orlitzky]

Just a guess since I deleted the patch file, but I think git merge uses a three-way merge strategy by default, whereas git am needs a special flag passed to it.