Repository navigation
Give the parsing threads more stack, and quiet a recovered Prism panic - #78
Merged
Merged
Conversation
Prism parses recursively, as the walk of its tree does, so each level of nesting in a file takes stack. A rayon thread has 2 MiB by default, which with Prism holds only about 1,400 to 2,500 levels of nested calls, hashes, arrays or blocks, where lib-ruby-parser held 16,000 or more. Past that, the whole run aborts with a stack overflow. Prism stops nesting after 10,000 levels and reports an error in the file. 16 MiB holds that many levels of each construct tried, where 8 MiB doesn't for arrays, parentheses, `!`, splats or indexes. A thread's stack is reserved, not committed, so peak memory on two large Rails apps is unchanged. A debug build takes more than ten times the stack per level, so the test's 500 levels overflow 2 MiB there, but not 16 MiB.
pks catches the panic that `Diagnostic::message` raises on a message that isn't UTF-8, but Rust's default panic hook had already printed it, with a path into the cargo registry that ruby-prism was built from, so the run looked as if it had crashed. A hook now leaves out a panic raised while a message is read, on that thread only, and passes any other to the hook it replaced.
rayon runs a `par_iter` that won't split on the thread that calls it, so a command with one file to parse, as `check <file>` and `check-contents` have, parsed it on the main thread. That thread has 8 MiB on macOS and Linux and 1 MiB on Windows, so in a release build a file nested about 9,990 deep still aborted pks. `cli::run` now runs the command on a thread with the same 16 MiB as the pool's. It keeps the global pool and spawns a thread, rather than running the command inside a pool with `install`, so that the command doesn't hold one of the pool's threads. With `RAYON_NUM_THREADS=1`, jwalk would wait a second for the busy pool and then walk nothing. The comment on the stack size now says it holds 10,000 levels in a release build, where Prism stops nesting most constructs. A debug build overflows 16 MiB at about 1,350 levels, and nested `case`/`in` patterns and long method chains go past Prism's cap. The new test runs `check` on one file nested 1,000 deep, which overflows an 8 MiB thread in a debug build.
A hook that swallowed every panic, or a flag left set after reading a message, passed the tests, since none of them checked that a panic still prints. The new test reads a message that panics and then panics itself, in a child of the test binary since the hook is the process's, and checks that only the second panic printed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two fixes from a review of #73 after it merged. Neither problem has been released, since #73 didn't bump the version.
fatal runtime error: stack overflow. lib-ruby-parser held 16,000 levels or more. The global thread pool's threads now get 16 MiB, and so does the thread the command runs on, since rayon parses a lone file, ascheck <file>andcheck-contentshave, on the thread that asks.Diagnostic::messageraises on a message that isn't UTF-8, but Rust's default panic hook printed it first, with a path into the cargo registry, so the run looked as if it had crashed. Now the warning is all that prints.Design notes
!, splats and indexes.elsifchains are flat in Prism's grammar but deep in its tree, and they overflowed 2 MiB only past about 16,000 levels; with 16 MiB, a chain of 100,000 links parses and one of 200,000 aborts. Nestedcase/inarray patterns abort between 30,000 and 40,000 levels, and hash patterns between 20,000 and 30,000. A real fix for those needs walks that don't recurse.checkandcheck-contentseven with 16 MiB on the pool.cli::runkeeps the global pool and spawns a thread for the command, rather than running the command in a pool withinstall, so that the command doesn't hold one of the pool's threads: withRAYON_NUM_THREADS=1, jwalk would wait a second for the busy pool and then walk nothing.Diagnostic::messagecould replace invalid bytes instead of panicking.list-referencestest nests 500 and thechecktest 1,000.Test plan
list-referenceson 500 nested arrays, under both parsers. It aborts without the pool's stack.checkon one file of 1,000 nested arrays, under both parsers. It aborts on the main thread without the command's thread.check <file>,check-contentsandlist-referenceson a file nested 9,990 deep finish under both parsers.cargo fmt --check,cargo clippy --all-targets --all-features -- -Dwarningsand the full suite pass.