Skip to content
  • Daniel Colson's avatar
    f3530578
    Introduce Journey::Ast to avoid extra ast walks · f3530578
    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: default avatarEric Milford <ericmilford@gmail.com>
    f3530578
    Introduce Journey::Ast to avoid extra ast walks
    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: default avatarEric Milford <ericmilford@gmail.com>
Loading