Reduce memory allocations for RI and POT generators - #1788
Conversation
55caf23 to
e89f08c
Compare
RDoc currently syntax-highlights and retains every Ruby method body during parsing, regardless of whether the selected generator renders method source. This PR adds a generator capability for controlling method-source collection. RI and POT opt out, while Darkfish, Aliki, and unknown third-party generators retain the existing behavior. This reduces roughly 50% of allocations for generators that opt-out.
e89f08c to
afe4edb
Compare
| def store_method_source? | ||
| return false if @coverage_report | ||
|
|
||
| !@generator.respond_to?(:store_method_source?) || @generator.store_method_source? |
There was a problem hiding this comment.
We need this respond_to? for custom generators 👍
But can we define store_method_source? = true for all existing generators in this repository?
| # Returns whether syntax-highlighted method source should be stored. | ||
|
|
||
| #: () -> bool | ||
| def store_method_source? |
There was a problem hiding this comment.
RDoc::RubyGemsHook#generate parses files once and then generates both formats from the same store, swapping the generator afterwards (simplified):
class RDoc::RubyGemsHook
def generate
@rdoc.store = RDoc::Store.new(parse_options)
@rdoc.parse_files parse_options.files
document 'ri', options, @ri_dir if @generate_ri and (@force or not File.exist? @ri_dir)
document 'aliki', options, @rdoc_dir if @generate_rdoc and (@force or not File.exist? @rdoc_dir)
end
endparse_options takes its generator from the gem's spec.rdoc_options. So if a gem specifies --format=ri there, parsing skips method source tokens, and the subsequent aliki generation raises:
method source for Foo#bar was not stored; set store_method_source? to trueReproduction: https://gist.github.com/tompng/5fe25eaeb6cb53164a3c49374d330914
Since the generator can be swapped after parsing like this, deriving the decision from options.generator at parse time is not reliable. I think store_method_source should be an explicit option that RubyGemsHook sets before parsing, e.g.:
parse_options.store_method_source = Generator::Aliki.store_method_source? || Generator::RI.store_method_source?
@rdoc.parse_files parse_options.files(strictly, only the generators that will actually run need to be OR-ed)
def store_method_source?
# return true or false if it's explicitly set
return @store_method_source unless store_method_source.nil?
# fallback path (@coveragage_report, @generator.store_method_source?)`
endThere was a problem hiding this comment.
Good catch!
I didn't know this was even possible. I'll take a closer look
There was a problem hiding this comment.
I'm closing this PR, suggested approach is not workable.
I'm leaning toward changing from eager code highlight approach to lazy one and working on a prototype in this PR:
#1792
Please see the prior PR #1788 In that PR it was correctly established that eager generation of `RDoc::Parser::RubyColorizer::ColoredToken` objects creates a heavy performance/system burden for all generators, but not all generators need these objects. My suggested approach in that PR was to establish a generator property that would stop this eager code colorization process. This approach is not functional, because rdoc itself can switch generators, but use the same @store object. In this PR I want to suggest an alternative solution: - Instead of doing this eagerly, do code colorization lazily (only if stream_token was actually called). - This would also ensure, that no new properties or methods will be introduced. And this will help us keep backward compatibility, while improving performance. ## Solution This change stores a copied method-source slice in a deferred token stream and materializes the existing mutable token array only when token_stream is accessed. It also switches the initial file scan from Prism.parse_lex to Prism.parse, preserves indentation and heredoc boundaries, and keeps the existing public token-stream behavior for HTML and third-party generators. ## Benchmark I have done a benchmark on google-client-api gem (one of the biggest and popular gems in the ecosystem). This could be found here: https://gist.github.com/skatkov/7e509742651586c1b993a8b15b10a8da ### RI #### After Parse | Variant | RSS | Peak RSS | Colored | Deferred | Raw strings | Raw source | Prism nodes | Prism tokens | Heap slots | | --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | | released | 958.8 MiB | 965.6 MiB | 3900930 | 0 | 0 | 0.0 MiB | 0 | 0 | 9998568 | | current | 478.2 MiB | 478.2 MiB | 0 | 46571 | 617 | 57.0 MiB | 0 | 0 | 2433708 | #### After Generate | Variant | RSS | Peak RSS | Colored | Deferred | Raw strings | Raw source | Prism nodes | Prism tokens | Heap slots | | --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | | released | 1356.7 MiB | 1391.4 MiB | 3900930 | 0 | 0 | 0.0 MiB | 0 | 0 | 11901244 | | current | 719.7 MiB | 755.3 MiB | 0 | 46571 | 617 | 57.0 MiB | 0 | 0 | 4336429 | | Variant | Parse | Generate | Total | GNU peak RSS | | --- | ---: | ---: | ---: | ---: | | released | 24.31s | 30.40s | 58.18s | 1391.4 MiB | | current | 14.62s | 28.59s | 45.43s | 755.3 MiB | Parse RSS change: -50.1% Generated RSS change: -47.0% Peak RSS change: -45.7% Parse time change: -39.8% Generate time change: -5.9% Total time change: -21.9% Outputs identical: yes ### Aliki, 100 files ```sh ./benchmark.rb --format aliki --limit 100 ``` #### After Parse | Variant | RSS | Peak RSS | Colored | Deferred | Raw strings | Raw source | Prism nodes | Prism tokens | Heap slots | | --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | | released | 96.3 MiB | 96.9 MiB | 253392 | 0 | 0 | 0.0 MiB | 0 | 0 | 708268 | | current | 65.4 MiB | 65.4 MiB | 0 | 2701 | 2701 | 0.9 MiB | 0 | 0 | 206955 | #### After Generate | Variant | RSS | Peak RSS | Colored | Deferred | Raw strings | Raw source | Prism nodes | Prism tokens | Heap slots | | --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | | released | 219.1 MiB | 220.4 MiB | 253392 | 0 | 0 | 0.0 MiB | 0 | 0 | 1037598 | | current | 210.3 MiB | 210.3 MiB | 253392 | 0 | 0 | 0.0 MiB | 0 | 0 | 1038957 | | Variant | Parse | Generate | Total | GNU peak RSS | | --- | ---: | ---: | ---: | ---: | | released | 1.40s | 44.09s | 46.10s | 220.4 MiB | | current | 0.90s | 44.56s | 46.04s | 209.5 MiB | Parse RSS changed by -32.1%, generated RSS by -4.0%, peak RSS by -4.9%, parse time by -35.7%, generation time by +1.1%, and total time by -0.1%. Outputs were identical. ## Results According to the benchmark, we can see that the lazy approach brings significant memory improvements even for generators that actually output highlighted code. In cases of 'RI', generator that has no use for highlighted code, we can see significant memory improvements (~50%), but also ~20% speed improvements. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Each documented method currently retains an array of
RDoc::Parser::RubyColorizer::ColoredTokenobjects and their fragmented source strings.This data is unnecessary for:
Large generated SDKs can contain millions of tokens. Avoiding those allocations substantially reduces parsing time and memory without changing generated output.
Implementation
Generators can declare whether they require method source:
The Ruby parser checks that capability before syntax highlighting a method body.
Generators without the capability default to true, preserving compatibility with existing third-party generators.
This also allows third-party generators (like rdoc-markdown) to skip this costly process if they don't have need for
method.token_streamormethod.markup_code.If
store_method_source?was disabled, buttoken_streamwas still accessed, an error will be raised.Benchmark
Here is a code that was used for benchmark this fix:
https://github.com/skatkov/rdoc-store-method-source-benchmark
RDoc 8.0.0 and
google-api-client0.53.0 on Ruby 4.0.6: