roo-rb / roo

Roo provides an interface to spreadsheets of several sorts.
MIT License
2.79k stars 501 forks source link

Reduce array allocations in create_cell_from_value #470

Closed will89 closed 6 years ago

will89 commented 6 years ago

Summary

This PR removes the array allocation for excelx_type in create_cell_from_value. Based on the comments from https://github.com/roo-rb/roo/pull/436#issuecomment-429220926.

Instead of memoizing the arrays as suggested in the comment, I went ahead and updated the interface to not require passing an array in the first place. It looks like all of the code in this gem just discarded the array and took the last element. Let me know if you think it's worthwhile for backwards compatibility in the cell constructors that called .last are worth updating with some logic like: @format = excelx_type.is_a?(Array) ? excelx_type.last : excelx_type.

Memory profiling against the file test/files/Bibelbund.xlsx

Master
Total allocated: 40143162 bytes (497194 objects)
Total retained:  1296083 bytes (11650 objects)

allocated memory by gem
-----------------------------------
  18683065  roo-rb-roo/lib
  11231729  nokogiri-1.8.5
  10226503  rubyzip-1.2.2
      1433  tmpdir
       240  weakref
       192  other

This PR
Total allocated: 39063762 bytes (470209 objects)
Total retained:  1295107 bytes (11631 objects)

allocated memory by gem
-----------------------------------
  17603665  will89-roo/lib
  11231729  nokogiri-1.8.5
  10226503  rubyzip-1.2.2
      1433  tmpdir
       240  weakref
       192  other

This PR used about ~6% less memory in the roo gem when iterating through the file.

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  #gem "roo", path: "/Users/gturner/development/roo"
  gem "roo", path: '/Users/will/repos/will89-roo'
  #gem "roo", path: '/Users/will/repos/roo-rb-roo'
  gem "minitest"
  gem "memory_profiler"
end

require "roo"
require "minitest/autorun"
require "memory_profiler"

class BugTest < Minitest::Test
  def test_stuff
    # add your test here
    report = MemoryProfiler.report do
      sheet = Roo::Spreadsheet.open("/Users/will/repos/will89-roo/test/files/Bibelbund.xlsx", no_hyperlinks: true)
      sheet.each_row_streaming do |row|
        row[1]
      end
    end
    report.pretty_print
  end
end
coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 94.085% when pulling 0e4d8eccfabb600ca0106e10e4979e2f1d2ac5bc on will89:feature/reduce-array-allocations into 8f65109c6728dd77f62b0a5c68dd42ab207dd914 on roo-rb:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 94.085% when pulling 0e4d8eccfabb600ca0106e10e4979e2f1d2ac5bc on will89:feature/reduce-array-allocations into 8f65109c6728dd77f62b0a5c68dd42ab207dd914 on roo-rb:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 94.085% when pulling 0e4d8eccfabb600ca0106e10e4979e2f1d2ac5bc on will89:feature/reduce-array-allocations into 8f65109c6728dd77f62b0a5c68dd42ab207dd914 on roo-rb:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 94.085% when pulling 0e4d8eccfabb600ca0106e10e4979e2f1d2ac5bc on will89:feature/reduce-array-allocations into 8f65109c6728dd77f62b0a5c68dd42ab207dd914 on roo-rb:master.

will89 commented 6 years ago

Yeah, big mistake on my part lol. I must have overlooked the base saving the excelx_type. Going to go ahead and close this. Thanks for catching that.