Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

comptime intepretter fails with stack-too-deep error from recursing when quicksorting a not-excessively large array #7213

Closed
jp4g opened this issue Jan 24, 2025 · 7 comments · Fixed by noir-lang/noir_sort#21 · May be fixed by noir-lang/sparse_array#15, #7214 or #7348
Assignees
Labels
bug Something isn't working

Comments

@jp4g
Copy link

jp4g commented Jan 24, 2025

Aim

Create a sparse array with a larger number of KV pairs in comptime

Expected Behavior

sparse_array::SparseArray::create will work with a large N value in comptime

Bug

Will throw

thread '<unknown>' has overflowed its stack
fatal runtime error: stack overflow
Aborted (core dumped)

To Reproduce

global ELEMENT_COUNT: u32 = 45; // this one works
// global ELEMENT_COUNT: u32 = 46;  // this will throw with...
// thread '<unknown>' has overflowed its stack
// fatal runtime error: stack overflow
// Aborted (core dumped)

global table: sparse_array::SparseArray<ELEMENT_COUNT, Field> = make_sparse_array();

comptime fn make_sparse_array() -> sparse_array::SparseArray<ELEMENT_COUNT, Field> {
    let mut indices: [Field; ELEMENT_COUNT] = [0; ELEMENT_COUNT];
    let mut values: [Field; ELEMENT_COUNT] = [0; ELEMENT_COUNT];
    for i in 0..ELEMENT_COUNT {
        indices[i] = (i) as Field;
        values[i] = (i * 2) as Field;
    }
    sparse_array::SparseArray::create(indices, values, (ELEMENT_COUNT * 2) as Field)
}

fn main(x: Field) {
    // just something to compile
    let y = table.get(x);
    assert(y == 0);
}

#[test]
fn test_main() {
    main(1);
}

run nargo compile with this library as a dependency in Nargo.toml

Workaround

None

Workaround Description

It works totally fine* at run time, but A: is still doing witcalc which takes a while and B: adds constraints for the user

Alternatively exploring building the SparseArray outside of Noir autogen

Finally could potentially recompile nargo with large RUST_MIN_STACK to get this done - however if this works it would still be prohibitive for 99% of developers

Additional Context

Appears that sparse_array::SparseArray::create -> sort_advanced -> quicksort_explicit is the culprit, with many

Project Impact

Blocker

Blocker Context

Blocks efficient use of zk-regex

Nargo Version

No response

NoirJS Version

1.0.0-beta.1+03b58fa2dfcc8acc8cf5198b1b23b55676fbdb02

Proving Backend Tooling & Version

0.66.0

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@jp4g jp4g added the bug Something isn't working label Jan 24, 2025
@jp4g
Copy link
Author

jp4g commented Jan 24, 2025

Not addressed by zkemail/zk-regex#75 (comment) btw

@TomAFrench TomAFrench transferred this issue from noir-lang/sparse_array Jan 28, 2025
TomAFrench added a commit that referenced this issue Jan 28, 2025
@TomAFrench
Copy link
Member

Hey, this isn't an issue with the library but with noir's comptime interpreter. I've then moved this issue across to the main noir repository.

@TomAFrench
Copy link
Member

This can be replicated with the programs

    global ELEMENT_COUNT: u32 = 100;
    comptime fn main() {
        let values: [u32; ELEMENT_COUNT] = [0; ELEMENT_COUNT];
        let _ = quicksort(values);
    }
    
    comptime fn partition<let N: u32>(
        arr: &mut [u32; N],
        low: u32,
        high: u32,
    ) -> u32 {
        let pivot = high;
        let mut i = low;
        for j in low..high {
            if arr[j] < arr[pivot] {
                let temp = arr[i];
                arr[i] = arr[j];
                arr[j] = temp;
    
                i += 1;
            }
        }
    
        let temp = arr[i];
        arr[i] = arr[pivot];
        arr[pivot] = temp;
        i
    }
    
    comptime fn quicksort_recursive<let N: u32>(
        arr: &mut [u32; N],
        low: u32,
        high: u32,
    ) {
        if low < high {
            let pivot_index = partition(arr, low, high);
            if pivot_index > 0 {
                quicksort_recursive(arr, low, pivot_index - 1);
            }
            quicksort_recursive(arr, pivot_index + 1, high);
        }
    }
    
    comptime fn quicksort<let N: u32>(
        _arr: [u32; N],
    ) -> [u32; N] {
        let mut arr: [u32; N] = _arr;
        if N > 1 {
            quicksort_recursive(&mut arr, 0, N - 1);
        }
        arr
    }    

or more minimally

    global RECURSION_LIMIT: u32 = 33;

    comptime fn main() {
        recurse(RECURSION_LIMIT);
    }
    
    comptime fn recurse(n: u32) {
        if n > 0 {
            recurse(n-1);
        }
    }

@TomAFrench TomAFrench changed the title Comptime sparse_array fails when N is too large comptime intepretter fails with stack-too-deep error from recursing when quicksorting a not-excessively large array Jan 30, 2025
@TomAFrench
Copy link
Member

@jfecher can you assign someone to look into this?

@jfecher
Copy link
Contributor

jfecher commented Jan 30, 2025

Reworking the interpreter in general to not stack overflow for a large* amount of recursive calls will be difficult I think. In general it requires queuing calls using a trampoline or similar. Since the interpreter would also need to preserve control-flow in the middle of a function it'd mean making it async. Since the interpreter also uses the elaborator... the entire noir frontend would need to be async.

For now we can maybe rework quicksort to use loops instead of recursion.

* "large" is obviously imprecise and I'd hope that ~46 recursive calls or however many were used wouldn't be considered such but the point stands that unless we're being very inefficient with stack memory in ways that are easily fixable, saving some memory here and there probably won't get us to 1000+ recursive calls.

@asterite
Copy link
Collaborator

asterite commented Feb 3, 2025

Would switching from a tree-walk interpreter to a VM-interpreter help here? For example here's part of the stack trace that repeats when it fails right now:

1497: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate_if
1498: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate_no_dereference
1499: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate
1500: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate_statement
1501: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate_block
1502: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate_no_dereference
1503: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate
1504: noirc_frontend::hir::comptime::interpreter::Interpreter::call_function_inner
1505: noirc_frontend::hir::comptime::interpreter::Interpreter::call_function
1506: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate_no_dereference
1507: noirc_frontend::hir::comptime::interpreter::Interpreter::evaluate

So a lot of function calls for interpreting something. I think with a VM-interpreter that would just be looping through bytecode, maybe pushing to a stack but here it's only one variable that's in the stack (and the "stack" could leave in the heap).

I tried using #[inline] in a few of the above functions and it helped a bit but it still crashes for relatively low numbers.

I don't know if a full-blown VM would be needed to solve this, maybe just transforming a function body or list of statements into bytecode so it can be looped instead of using function calls.

@michaeljklein
Copy link
Contributor

@asterite Rust doesn't have a way to ensure tail calls (see here), so I think that the idea of setting up a "trampoline" would end up being similar here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment