odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.12k stars 550 forks source link

Fix big.shrink not actually shrinking #3758

Closed jones-josh closed 2 weeks ago

jones-josh commented 2 weeks ago

Consider this program:

package main

import "core:fmt"
import "core:math/big"

main :: proc() {
    number := big.Int{}

    big.atoi(&number, "100")
    fmt.println(&number)

    fmt.println("before grow: ", len(number.digit), " / ", cap(number.digit))
    big.grow(&number, 128)
    fmt.println("before shrink: ", len(number.digit), " / ", cap(number.digit))
    big.shrink(&number)
    fmt.println("after shrink: ", len(number.digit), " / ", cap(number.digit))

    e := big.int_sub_digit(&number, &number, 1)
    fmt.println(e, &number)
}

Currently, it outputs the below. Notice the capacity after the "shrink". This capacity is checked against during the big.int_sub_digit as it calls grow again to ensure it will fit the possible new digit. (This could be replaced by any similar proc or just a grow call with the right params).

&Int{used = 1, digit = [100, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], sign = "Zero_or_Positive", flags = bit_set[Flag; u8]{}}
before grow:  32  /  32
before shrink:  128  /  128
after shrink:  3  /  128
Out_Of_Memory &Int{used = 1, digit = [100, 0, 0], sign = "Zero_or_Positive", flags = bit_set[Flag; u8]{}}

After the change:

&Int{used = 1, digit = [100, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], sign = "Zero_or_Positive", flags = bit_set[Flag; u8]{}}
before grow:  32  /  32
before shrink:  128  /  128
after shrink:  3  /  3
Okay &Int{used = 1, digit = [99, 0, 0], sign = "Zero_or_Positive", flags = bit_set[Flag; u8]{}}

(and a typo)

Kelimion commented 2 weeks ago

Good catch. I've run the test suite with this in place a dozen or so times and it consistently ran 3.5% faster without the shrink fix, but it's a good fix and the intended behavior, so it goes in.

jones-josh commented 2 weeks ago

Yeah thought I was going nuts when my subtract 1 wasn't working after a shrink. Shame about the regression though - I was kinda hoping reserve would have an allow_shrink flag so that wouldn't be a problem but even if it was added (not saying we should lol) it would only move that branch inwards.

Was the CI just having a moment?

Kelimion commented 2 weeks ago

Was the CI just having a moment?

It has these senior moments occasionally where it ends up at the fridge and forgets which dessert it wanted, only to forget entirely it wasn't done running our tests yet.

jones-josh commented 2 weeks ago

Hmm just realised that the && allow_shrink isn't actually necessary since needed is at least as large as cap. Think it's worth removing? It won't get rid of the branch and makes it a little less obvious that that's the shrinkage case. Maybe LLVM IR optimisations catch it anyway?

jasonKercher commented 2 weeks ago

shinkage