-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
Improved handling for cycles within the query system itself #154387
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
base: main
Are you sure you want to change the base?
Changes from all commits
193126a
bcb2621
4d128c0
95308ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,9 @@ | ||
| use std::hint::{likely, unlikely}; | ||
| use std::io::{self, Write}; | ||
| use std::sync::atomic::{AtomicU16, Ordering}; | ||
|
|
||
| use backtrace::{Backtrace, BacktraceFrame}; | ||
|
|
||
| // This is the amount of bytes that need to be left on the stack before increasing the size. | ||
| // It must be at least as large as the stack required by any code that does not call | ||
| // `ensure_sufficient_stack`. | ||
|
|
@@ -11,12 +17,92 @@ const STACK_PER_RECURSION: usize = 1024 * 1024; // 1MB | |
| #[cfg(target_os = "aix")] | ||
| const STACK_PER_RECURSION: usize = 16 * 1024 * 1024; // 16MB | ||
|
|
||
| thread_local! { | ||
| static TIMES_GROWN: AtomicU16 = const { AtomicU16::new(0) }; | ||
| } | ||
|
|
||
| // Give up if we expand the stack this many times and are still trying to recurse deeper. | ||
| const MAX_STACK_GROWTH: u16 = 1000; | ||
|
|
||
| /// Grows the stack on demand to prevent stack overflow. Call this in strategic locations | ||
| /// to "break up" recursive calls. E.g. almost any call to `visit_expr` or equivalent can benefit | ||
| /// from this. | ||
| /// | ||
| /// Should not be sprinkled around carelessly, as it causes a little bit of overhead. | ||
| #[inline] | ||
| pub fn ensure_sufficient_stack<R>(f: impl FnOnce() -> R) -> R { | ||
| stacker::maybe_grow(RED_ZONE, STACK_PER_RECURSION, f) | ||
| // if we can't guess the remaining stack (unsupported on some platforms) we immediately grow | ||
| // the stack and then cache the new stack size (which we do know now because we allocated it. | ||
| let enough_space = match stacker::remaining_stack() { | ||
| Some(remaining) => remaining >= RED_ZONE, | ||
| None => false, | ||
| }; | ||
| if likely(enough_space) { | ||
| f() | ||
| } else { | ||
| let times = TIMES_GROWN.with(|times| times.fetch_add(1, Ordering::Relaxed)); | ||
| if unlikely(times > MAX_STACK_GROWTH) { | ||
| too_much_stack(); | ||
| } | ||
| let out = stacker::grow(STACK_PER_RECURSION, f); | ||
| TIMES_GROWN.with(|times| times.fetch_sub(1, Ordering::Relaxed)); | ||
| out | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure this new implementation breaks on targets where stacker uses the default impl of not growing the stack. There |
||
| } | ||
|
|
||
| #[cold] | ||
| fn too_much_stack() -> ! { | ||
| let Err(e) = std::panic::catch_unwind(report_too_much_stack); | ||
| let mut stderr = io::stderr(); | ||
| let _ = writeln!(stderr, "ensure_sufficient_stack: panicked while handling stack overflow!"); | ||
| if let Ok(s) = e.downcast::<String>() { | ||
| let _ = writeln!(stderr, "{s}"); | ||
| } | ||
| std::process::abort(); | ||
| } | ||
|
|
||
| #[cold] | ||
| fn report_too_much_stack() -> ! { | ||
| // something is *definitely* wrong. | ||
| eprintln!( | ||
| "still not enough stack after {MAX_STACK_GROWTH} expansions of dynamic stack; infinite recursion?" | ||
| ); | ||
|
|
||
| let backtrace = Backtrace::new_unresolved(); | ||
| let frames = backtrace.frames(); | ||
| eprintln!("first hundred frames:"); | ||
| print_frames(0, &frames[..100]); | ||
|
|
||
| eprintln!("...\nlast hundred frames:"); | ||
| let start = frames.len() - 100; | ||
| print_frames(start, &frames[start..]); | ||
| std::process::abort(); | ||
| } | ||
|
|
||
| #[cold] | ||
| fn print_frames(mut i: usize, frames: &[BacktraceFrame]) { | ||
| for frame in frames { | ||
| let mut frame = frame.clone(); | ||
| frame.resolve(); | ||
| for symbol in frame.symbols() { | ||
| eprint!("{i}: "); | ||
| match symbol.name() { | ||
| Some(sym) => eprint!("{sym}"), | ||
| None => eprint!("<unknown>"), | ||
| } | ||
| eprint!("\n\t\tat "); | ||
| if let Some(file) = symbol.filename() { | ||
| eprint!("{}", file.display()); | ||
| if let Some(line) = symbol.lineno() { | ||
| eprint!(":{line}"); | ||
| if let Some(col) = symbol.colno() { | ||
| eprint!(":{col}"); | ||
| } | ||
| } | ||
| } else { | ||
| eprint!("<unknown>"); | ||
| } | ||
| eprintln!(); | ||
| i += 1; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be
Cell<u16>, right?