Commit Graph

2 Commits

Author SHA1 Message Date
Daniel Colson f35305785d
Introduce Journey::Ast to avoid extra ast walks
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 a9336a67b0 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>
2021-07-29 16:23:11 -04:00
Daniel Colson 8eec6ee962
Move Path::Pattern factories into test helper
`Pattern#from_sting` was introduced in bb207ea7 to support creating
patterns from strings in tests (removing a conditional in the initialize
method that had been there only for the sake of those tests).

`Pattern#build` was introduced in 947ebe9a, and was similarly used only
in tests.

My understanding is that [`Mapping#path`][path] is the only place where
we initialize a `Path::Pattern` in library code.

Since these two methods appear to be used only in tests, this
commit moves them out of the class and into a test helper.

My reasoning for doing this is that I am doing some performance work
that may involve a modification to how `Path::Pattern`s get initialized,
and I will have more confidence in that work if I know for sure that
these methods are test helpers that can be modified freely.

[path]: https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/mapper.rb#L192-L194
2020-07-27 12:23:33 -04:00