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

Cache author info in Comment classes #465

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jpslav
Copy link

@jpslav jpslav commented Apr 30, 2016

For a spreadsheet with 420 rows and around 150 columns, we were seeing Axlsx serialization times of around 317 seconds. I tracked the problem down to the Axlsx Comment and Comments classes always recalculating author lists and indexes of authors. Our workbook had lots of comments and so these recalculations that might otherwise go unnoticed stood out. In a ruby-prof run, Axlsx::Comments#to_xml_string was taking around 90-95% of the total run time.

Since we have no need to indicate comment authors in our workbooks, I initially monkey patched two methods in Axlsx to fix this:

module Axlsx
  class Comments < SimpleTypedList
    def authors
      [""]
    end
  end

  class Comment
    def author_index
      0
    end
  end
end

This change took our Axlsx time down to 5 seconds.

This PR achieves the same time savings but preserves the existing capabilities by introducing caching into the sorted authors list and into the method that returns the index of an author in that list.

@@ -61,7 +61,6 @@ def ref=(v)
# @param [String] str
# @return [String]
def to_xml_string(str = "")
author = @comments.authors[author_index]
Copy link
Author

@jpslav jpslav Apr 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this line because I'm pretty sure author is already an attribute of this Comment class.

@jpslav jpslav force-pushed the comment-author-caching branch from ae7782b to 1be29d6 Compare April 30, 2016 05:28
@jpslav jpslav force-pushed the comment-author-caching branch from 1be29d6 to 9c9bbc8 Compare April 30, 2016 05:28
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.

1 participant