Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stash and retrieve computed regex from a cache #598

Closed
wants to merge 3 commits into from

Conversation

ashmaroli
Copy link
Contributor

@ashmaroli ashmaroli commented May 6, 2019

Summary

Use a memoized cache to stash and retrieve computed regular expressions to avoid allocating duplicate regexes.

Context

Results from profiling Jekyll documentation source with memory_profiler

Before

Total allocated: 158.08 MB (1168224 objects)
Total retained:  10.3 MB (92547 objects)

After

Total allocated: 156.61 MB (1161925 objects)
Total retained:  10.3 MB (92548 objects)

@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented May 23, 2019

Profiler Diff Summary

--- master branch https://travis-ci.org/gettalong/kramdown/jobs/536192573
+++ PR branch     https://travis-ci.org/gettalong/kramdown/jobs/536298613

- Total allocated: 220.89 MB (1783975 objects)
- Total retained:  1.11 MB (1582 objects)
+ Total allocated: 210.65 MB (1737868 objects)
+ Total retained:  1.11 MB (1583 objects)

@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented Jul 1, 2019

🔔 Ding-Dong @gettalong
Just in case you forgot about this....

@gettalong
Copy link
Owner

@gettalong gettalong commented Jul 1, 2019

No, didn't forget, just didn't have time...

@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 17, 2020

@ashmaroli Besides a 4,5% memory improvement and a 2,5% object allocation improvement, is it faster CPU-wise?

@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented Apr 18, 2020

I'm not sure how to benchmark this appropriately, both stop_re and span_start are computed at runtime.
The best way would be to benchmark master vs pr-branch with an appropriate source string to be parsed during the benchmark....

Was hoping you could provide me with the appropriate sample content that covers multiple use-cases...

Until then, we could benchmark just the change..

Disclaimer: Micro-benchmarking such as this may be flawed because it disregards the overhead from rest of the codebase:

require 'benchmark/ips'

class KParser
  # Based purely on the two expressions generated while parsing Jekyll's docs
  STOP_RE = /(\])|!?\[/
  SPAN_START = /\*|_|`|<|<|\[|!?\[|[^\\]?["']|\$|\{:|&|--|\.\.\.|(?:\\| )?(?:<<|>>)|(  |\\)(?=\n)|\\|~~/

  def computed_pattern
    /(?=#{Regexp.union(STOP_RE, SPAN_START)})/
  end

  def stashed_pattern
    @stashed_pattern ||= {}
    @stashed_pattern[STOP_RE] ||= {}
    @stashed_pattern[STOP_RE][SPAN_START] ||= /(?=#{Regexp.union(STOP_RE, SPAN_START)})/
  end
end

parser = KParser.new
return unless parser.computed_pattern == parser.stashed_pattern

Benchmark.ips do |x|
  x.report('computed_pattern') { parser.computed_pattern }
  x.report('stashed_pattern')  { parser.stashed_pattern }
  x.compare!
end

@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 19, 2020

Okay, so I checked how often the regexp would be compiled when generating the kramdown documentation, 1423 times. I didn't notice any runtime differences.

Nonetheless, I think it's okay to merge this since the meaning of the change is quite clear when reading the code.

One thing: Please move the creation of the hash into the initialization code with something like this: Hash.new {|h, k| h[k] = {}}. And probably rename it to span_pattern_cache.

@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented Apr 20, 2020

initialization code with something like this: Hash.new {|h, k| h[k] = {}}

I think you're directing me to use an autovivifying hash:

@cache ||= Hash.new { |h, k| h[k] = Hash.new(&h.default_proc) }
@cache[foo][bar] ||= result

However, from what I recall about profiling memory usage, the autovivified hash uses more memory (about 4-6x) than {} (I'm guessing the additional allocation is for the procs..)

I'll try and rerun some tests..

@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 20, 2020

Oh, we don't need the recursive auto-hash. We only need two levels, one for stop_re and one for span_start, so the code shown in my comment should be enough!

@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented Apr 20, 2020

Apologies for jumping the gun back there. I've made the requested changes.
In a future pull request, I may stash the { |h, k| h[k] = {} } in a private constant so that the Proc isn't initialized for every parser instance.. but for the time being, it seems okay..

Thanks.

@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 20, 2020

I thought about using a constant, too, but didn't mention it because I don't think there is much additional benefit to it.

@gettalong gettalong self-assigned this Apr 21, 2020
@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 21, 2020

Thanks for the changes! I have merged them and moved the initialization of the hash into the class initializer - no need to perform that operation everytime.

@gettalong gettalong closed this Apr 21, 2020
@ashmaroli
Copy link
Contributor Author

@ashmaroli ashmaroli commented Apr 21, 2020

no need to perform that operation everytime

The ||= operator ensures that the RHS expression is only executed when the LHS expression evaluates to a falsy.
That said, I'm not against having the logic moved into the constructor, just don't see the difference since the result is stashed in an instance variable anyway.

Thanks for merging the change.
Is there an ETA on the next kramdown release?

@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 21, 2020

The only difference is that the check of the LHS expression being truthy doesn't need to be done. So instructions are saved resulting in faster execution.

As for the ETA: One last look through the open issues/PRs and then I will release the new version. So either today if I find the time, else tomorrow.

@ashmaroli ashmaroli deleted the cache-computed-regex branch Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants