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 default options in a constant / memoized variable #685

Closed
ashmaroli opened this issue Aug 16, 2020 · 5 comments
Closed

Stash default options in a constant / memoized variable #685

ashmaroli opened this issue Aug 16, 2020 · 5 comments
Assignees

Comments

@ashmaroli
Copy link
Contributor

Currently when a Document is parsed and converted, the default options hash are freshly generated:

@options = Kramdown::Options.merge(options)

converter = new(tree, ::Kramdown::Options.merge(options.merge(tree.options[:options] || {})))

# Merge the #defaults Hash with the *parsed* options from the given Hash, i.e. only valid option
# names are considered and their value is run through the #parse method.
def self.merge(hash)
temp = defaults

# Return a Hash with the default values for all options.
def self.defaults
temp = {}
@options.each {|_n, o| temp[o.name] = o.default }
temp
end

When numerous Documents, say about 1000, are parsed and converted in a single process, there's a high probability of about 2k temporary hash objects getting allocated.

Suggestion

Since Options.merge invariably generates a new Hash, it would be nice if the default options were a single object for the entire duration of the process.

@gettalong gettalong self-assigned this Jan 6, 2021
@gettalong
Copy link
Owner

As can be seen in the last of your snippets, the Kramdown::Options.defaults method creates a new hash. You propose to make this method always return the same hash, am I right?

If we would do this, we would need to create a new hash in Kramdown::Options.merge:

    def self.merge(hash)
      temp = defaults
      hash.each do |k, v|
        k = k.to_sym
        temp[k] = @options.key?(k) ? parse(k, v) : v
      end
      temp
    end

As you can see, the first line just uses the return value of ::defaults. Later in the method that hash is modified by assigning values to keys.

This means if we don't create a new hash in ::defaults, we would need to use #dup in ::merge. I don't see how we could avoid this.

@ashmaroli
Copy link
Contributor Author

This means if we don't create a new hash in ::defaults, we would need to use #dup in ::merge.

Yes, I realize the need to generate a new Hash object.

I don't see how we could avoid this.

I actually prefer using Hash#dup in ::merge if the result is absolutely the same because then there wouldn't be repeated iteration through @options with additional computations in the enumerator's block only to return the same Hash
(contents-wise) repeatedly.

@gettalong
Copy link
Owner

Ah, thanks, yes, this makes sense. If we go this way, we will need to clear the cached value if new option definitions are added later on. I will let this sink in and do the appropriate changes in the next few days.

@ashmaroli
Copy link
Contributor Author

Thank you very much :)

@gettalong
Copy link
Owner

Took a bit longer than a few days but is implemented now and will be in the next release.

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

No branches or pull requests

2 participants