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

Reduce array allocations in Roo::Utils.each_element #466

Merged
merged 1 commit into from
Oct 14, 2018

Conversation

will89
Copy link
Contributor

@will89 will89 commented Oct 12, 2018

Summary

Taking some of the memory improvement changes from #436. This reduces the number of array allocations in Roo::Utils.each_element when the elements argument is not an Array.

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

Master
Total allocated: 42492082 bytes (555917 objects)
Total retained:  1295107 bytes (11631 objects)

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

This PR
Total allocated: 40143162 bytes (497194 objects)
Total retained:  1295107 bytes (11631 objects)

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

This PR used about ~16% less memory when iterating through the file and allocated ~10% fewer objects.

# 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/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
Copy link

Coverage Status

Coverage increased (+0.04%) to 94.098% when pulling 467f163 on will89:improve-utils-each_element into 5fc4378 on roo-rb:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 94.098% when pulling 467f163 on will89:improve-utils-each_element into 5fc4378 on roo-rb:master.

@chopraanmol1 chopraanmol1 merged commit 01d1a1c into roo-rb:master Oct 14, 2018
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 20, 2019
pkgsrc change: add "USE_LANGUAGES= # none".

##  [2.8.0] 2019-01-18
### Fixed
- Fixed inconsistent column length for CSV [375](roo-rb/roo#375)
- Fixed formatted_value with `%` for Excelx [416](roo-rb/roo#416)
- Improved Memory consumption and performance [434](roo-rb/roo#434) [449](roo-rb/roo#449) [454](roo-rb/roo#454) [456](roo-rb/roo#456) [458](roo-rb/roo#458) [462](roo-rb/roo#462) [466](roo-rb/roo#466)
- Accept both Transitional and Strict Type for Excelx's worksheets [441](roo-rb/roo#441)
- Fixed ruby warnings [442](roo-rb/roo#442) [476](roo-rb/roo#476)
- Restore support for URL as file identifier for CSV [462](roo-rb/roo#462)
- Fixed missing location for Excelx's links [482](roo-rb/roo#482)

### Changed / Added
- Drop support for ruby 2.2.x and lower
- Updated rubyzip version for fixing security issue. Now minimal version is 1.2.1
- Roo::Excelx::Coordinate now inherits Array [458](roo-rb/roo#458)
- Improved Roo::HeaderRowNotFoundError exception's message [461](roo-rb/roo#461)
- Added `empty_cell` option which by default disable allocation for Roo::Excelx::Cell::Empty [464](roo-rb/roo#464)
- Added support for variable number of decimals for Excelx's formatted_value [387](roo-rb/roo#387)
- Added `disable_html_injection` option to disable html injection for shared string in `Roo::Excelx` [392](roo-rb/roo#392)
- Added image extraction for Excelx [414](roo-rb/roo#414) [397](roo-rb/roo#397)
- Added support for `1e6` as scientific notation for Excelx [433](roo-rb/roo#433)
- Added support for Integer as 0 based index for Excelx's `sheet_for` [455](roo-rb/roo#455)
- Extended `no_hyperlinks` option for non streaming Excelx methods [459](roo-rb/roo#459)
- Added `empty_cell` option to disable Roo::Excelx::Cell::Empty allocation for Excelx [464](roo-rb/roo#464)
- Added support for Integer with leading zero for Roo:Excelx [479](roo-rb/roo#479)
- Refactored Excelx code [453](roo-rb/roo#453) [477](roo-rb/roo#477) [483](roo-rb/roo#483) [484](roo-rb/roo#484)

### Deprecations
- Roo::Excelx::Sheet#present_cells is deprecated [454](roo-rb/roo#454)
- Roo::Utils.split_coordinate is deprecated [458](roo-rb/roo#458)
- Roo::Excelx::Cell::Base#link is deprecated [457](roo-rb/roo#457)
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.

3 participants