-
Daniel Colson authored
This commit introduces a new `Journey::Ast` class that wraps the root node of an ast. The purpose of this class is to reduce the number of walks through the ast by taking a single pass through each node and caching values for later use. To avoid retaining additional memory, we clear out these ast objects after eager loading. Benefits --- * Performance improvements (see benchmarks below) * Keeps various ast manipulations together in a single class, rather than scattered throughout * Adding some names to things will hopefully make this code a little easier to follow for future readers Benchmarks --- We benchmarked loading a real routes file with > 3500 routes. master was at a9336a67 when we ran these. Note that these benchmarks also include a few small changes that we didn't include in this commit, but that we will follow up with after this gets merged - these additional changes do not change the benchmarks significantly. Time: ``` master - 0.798 ± 0.024 (500 runs) this branch - 0.695 ± 0.029 (500 runs) ``` Allocations: ``` master - 980149 total allocated objects this branch - 931357 total allocated objects ``` Stackprof: Seeing `ActionDispatch::Journey::Visitors::Each#visit` more frequently on the stack is what led us down this path in the first place. These changes seem to have done the trick. ``` master: TOTAL (pct) SAMPLES (pct) FRAME 52 (0.5%) 52 (0.5%) ActionDispatch::Journey::Nodes::Node#symbol? 58 (0.5%) 45 (0.4%) ActionDispatch::Journey::Scanner#scan 45 (0.4%) 45 (0.4%) ActionDispatch::Journey::Nodes::Cat#type 43 (0.4%) 43 (0.4%) ActionDispatch::Journey::Visitors::FunctionalVisitor#terminal 303 (2.7%) 43 (0.4%) ActionDispatch::Journey::Visitors::Each#visit 69 (0.6%) 40 (0.4%) ActionDispatch::Routing::Mapper::Scope#each this commit: TOTAL (pct) SAMPLES (pct) FRAME 82 (0.6%) 42 (0.3%) ActionDispatch::Journey::Scanner#next_token 31 (0.2%) 31 (0.2%) ActionDispatch::Journey::Nodes::Node#symbol? 30 (0.2%) 30 (0.2%) ActionDispatch::Journey::Nodes::Node#initialize ``` See also the benchmark script in https://github.com/rails/rails/pull/39935#issuecomment-887791294 Co-authored-by:
Eric Milford <ericmilford@gmail.com>
Daniel Colson authoredThis commit introduces a new `Journey::Ast` class that wraps the root node of an ast. The purpose of this class is to reduce the number of walks through the ast by taking a single pass through each node and caching values for later use. To avoid retaining additional memory, we clear out these ast objects after eager loading. Benefits --- * Performance improvements (see benchmarks below) * Keeps various ast manipulations together in a single class, rather than scattered throughout * Adding some names to things will hopefully make this code a little easier to follow for future readers Benchmarks --- We benchmarked loading a real routes file with > 3500 routes. master was at a9336a67 when we ran these. Note that these benchmarks also include a few small changes that we didn't include in this commit, but that we will follow up with after this gets merged - these additional changes do not change the benchmarks significantly. Time: ``` master - 0.798 ± 0.024 (500 runs) this branch - 0.695 ± 0.029 (500 runs) ``` Allocations: ``` master - 980149 total allocated objects this branch - 931357 total allocated objects ``` Stackprof: Seeing `ActionDispatch::Journey::Visitors::Each#visit` more frequently on the stack is what led us down this path in the first place. These changes seem to have done the trick. ``` master: TOTAL (pct) SAMPLES (pct) FRAME 52 (0.5%) 52 (0.5%) ActionDispatch::Journey::Nodes::Node#symbol? 58 (0.5%) 45 (0.4%) ActionDispatch::Journey::Scanner#scan 45 (0.4%) 45 (0.4%) ActionDispatch::Journey::Nodes::Cat#type 43 (0.4%) 43 (0.4%) ActionDispatch::Journey::Visitors::FunctionalVisitor#terminal 303 (2.7%) 43 (0.4%) ActionDispatch::Journey::Visitors::Each#visit 69 (0.6%) 40 (0.4%) ActionDispatch::Routing::Mapper::Scope#each this commit: TOTAL (pct) SAMPLES (pct) FRAME 82 (0.6%) 42 (0.3%) ActionDispatch::Journey::Scanner#next_token 31 (0.2%) 31 (0.2%) ActionDispatch::Journey::Nodes::Node#symbol? 30 (0.2%) 30 (0.2%) ActionDispatch::Journey::Nodes::Node#initialize ``` See also the benchmark script in https://github.com/rails/rails/pull/39935#issuecomment-887791294 Co-authored-by:
Eric Milford <ericmilford@gmail.com>
Loading