Skip to content

Loading…

More performance improvements... with a (subversive?) ulterior motive #395

Closed
wants to merge 5 commits into from

3 participants

@alexdowad

These optimizations were prompted by the thought that for a library which is widely used in the Ruby community, it sure would be nice to support more popular Ruby implementations. The message on "Supported Ruby Versions" in README.md, while written in a polite and friendly tone, sends a very clear message that "if you use anything but MRI, you're on your own". I feel that it should be possible to do better than that.

It seems to me that if it was possible to run the specs against all popular implementations every time a change was made, without taking up more of the maintainers' time, giving official sanction to more implementations would be an easy decision. As README.md points out, Prawn is pure Ruby, so the vast majority of changes should work "as is" on all implementations anyways.

To that end, I made a couple optimizations which speed up running the specs greatly. Also, I submitted a patch to the pdf-reader gem, which also makes a big difference. Since most developers are working on multi-core machines these days, I also added a script to the Rakefile which forks off a different process to run the specs against each implementation, so as to utilize all the available CPUs.

The net result (once a new version of pdf-reader is released) should be to make testing against 6 implementations take just a few seconds more than testing against 1 implementation did previously.

@alexdowad

When running the specs under JRuby (so as to use Java profiling tools) over 390MB of memory allocation was occurring at this line. I remember working on a Rails-based site which was crashing for lack of memory when using Prawn to process PNGs with transparency... it would be interesting to see how it would fare after applying this patch.

@alexdowad

Parsing AFM files was a major hotspot for the specs, since it was done for each new Prawn::Document, and the specs (naturally) create a lot of Document objects. I would expect that any web app which generates PDFs for download would benefit from this patch -- it will cause them to only read and parse AFM font files for the first request which comes in, and then just use the parsed data cached in memory, for as long as the app keeps running without a restart (which could be months or years).

@bradediger
prawnpdf member

Hi @alexdowad,
Thanks for the performance patches! In general, these look great and are much appreciated. One concern is that I'd like to see a more polymorphic approach to running multiple VMs; for example, I don't use rvm, rather rbenv. I'd like not to bake in the rvm assumption if possible.

@alexdowad
@bradediger
prawnpdf member

If we're going to bake in support for a Ruby version manager, testing for rvm and rbenv is probably the best approach. It would be nice to have a more abstract interface but I think that's probably a pipe dream.

@yob
prawnpdf member

My preference is not to depend on RVM or rbenv. I use neither on my systems, and I'd prefer not to add them.

Can we use on travis for testing across multiple VMs? I use it to ensure pdf-reader runs on {MRI/jruby/rubinius} in 1.8 and 1.9 modes.

@yob
prawnpdf member

travis does mean that I'm generally runinng the tests locally against MRI 1.9.3 and then only after pushing to github do I find the specs are broken on another VM. In practice I haven't found that to be an issue.

@alexdowad
@yob
prawnpdf member

I've added prawn to travis-ci as a trial, you can see it at http://travis-ci.org/#!/prawnpdf/prawn

@alexdowad

Some of these instance variables were previously set as a side effect of parsing an AFM file. Since we are caching and reusing the parsed AFM data, we need to set them here instead.

@alexdowad

This and the following calculation were previously done on demand, and the results stored in instance variables. I moved them here so they can be done only once for each AFM file.

@alexdowad

I'm guessing that the Prawn team is happy using Travis for testing against alternate Ruby implementations. If that is so, perhaps 9282fae, 222d268, and b3b2199 can be cherry-picked from this PR before it is closed?

@yob
prawnpdf member

+1

I'm on holidays at the moment and have some time up my sleeve. If everyone agrees with the suggestion, I'm happy to complete this.

@bradediger
prawnpdf member

@yob I'd love it if you are able to knock this out. Everything looks good to me.

@yob
prawnpdf member

Done. Travis shows us red on rbx 1.9 and jruby 1.9, green on everything else.

@yob yob closed this
@yob
prawnpdf member

I've removed rbx1.9 from the travis targets for now as rbx is a moving target.

jruby1.9 is still failing with what seem like legitimate encoding failures, although I'm not sure why they don't occur on MRI. A job for another day.

@alexdowad
@alexdowad
@yob
prawnpdf member

Which error are you looking at? I'm looking at the encoding errors at http://travis-ci.org/#!/prawnpdf/prawn/jobs/2551473

@alexdowad
@alexdowad
@yob
prawnpdf member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 175 additions and 56 deletions.
  1. +1 −0 .gitignore
  2. +8 −1 README.md
  3. +68 −10 Rakefile
  4. +1 −0 lib/prawn.rb
  5. +36 −39 lib/prawn/font/afm.rb
  6. +12 −4 lib/prawn/images/png.rb
  7. +44 −0 lib/prawn/utilities.rb
  8. +5 −2 spec/png_spec.rb
View
1 .gitignore
@@ -7,3 +7,4 @@ Gemfile.lock
drop_to_console.rb
manual.pdf
/doc
+*.rbc
View
9 README.md
@@ -23,7 +23,14 @@ One thing Prawn is not, and will never be, is an HTML to PDF generator. For thos
## Supported Ruby Versions and Implementations
-Because Prawn is pure Ruby and virtually all of its dependencies are maintained by our core team, it should run pretty much anywhere, including Rubinius, JRuby, MacRuby, etc. While we officially support MRI 1.8.7 and 1.9.2 only, we will accept patches to fix problems on other Ruby platforms if they aren't too invasive.
+Because Prawn is pure Ruby and virtually all of its dependencies are maintained by our core team, it should run pretty much anywhere. The latest version has been tested against:
+
+* MRI 1.8.7
+* MRI 1.9.2
+* MRI 1.9.3
+* JRuby 1.6.7
+* Rubinius 1.2.4
+* REE 1.8.7
## Installing Prawn
View
78 Rakefile
@@ -1,15 +1,15 @@
-require "rubygems"
-require "bundler"
+require 'rubygems'
+require 'bundler'
Bundler.setup
require 'rake'
require 'rake/testtask'
-require "rake/rdoctask"
-require "rake/gempackagetask"
+require 'rake/rdoctask'
+require 'rake/gempackagetask'
task :default => [:test]
-desc "Run all tests, test-spec, mocha, and pdf-reader required"
+desc "Run all tests (test-spec, mocha, and pdf-reader required)"
Rake::TestTask.new do |test|
# test.ruby_opts << "-w" # .should == true triggers a lot of warnings
test.libs << "spec"
@@ -17,15 +17,15 @@ Rake::TestTask.new do |test|
test.verbose = true
end
-desc "Show library's code statistics"
+desc "Show code statistics"
task :stats do
- require 'code_statistics'
+ require 'code_statistics'
CodeStatistics::TEST_TYPES << "Specs"
- CodeStatistics.new( ["Prawn", "lib"],
- ["Specs", "spec"] ).to_s
+ CodeStatistics.new( ["Prawn", "lib"],
+ ["Specs", "spec"] ).to_s
end
-desc "genrates documentation"
+desc "Generate documentation"
Rake::RDocTask.new do |rdoc|
rdoc.rdoc_files.include( "README",
"COPYING",
@@ -49,3 +49,61 @@ Rake::GemPackageTask.new(spec) do |pkg|
pkg.need_zip = true
pkg.need_tar = true
end
+
+desc "Run all tests on all supported VMs (rvm required)"
+task :test_all_vms do
+ supported = %w{ruby-1.9.3-p194
+ ruby-1.9.2-p320
+ ruby-1.8.7-p370
+ jruby-1.6.7.2
+ rbx-1.2.4
+ ree-1.8.7}
+
+ unless `rvm version` =~ /rvm \d+\.\d+/
+ puts "\nPlease ensure that rvm is correctly installed and try again."
+ exit
+ end
+
+ installed = `rvm list`
+ unless (to_install = supported.reject { |vm| installed.match(vm) }).empty?
+ puts "\nPlease install the following Ruby implementation#{'s' unless to_install.one?} (using rvm) and try again:"
+ puts to_install.map { |vm| " #{vm}" }
+ exit
+ end
+
+ # So you dropped big $$$ on an expensive dev machine with tons of CPU cores?
+ # Let's put those cores to work...
+ puts "Starting tests..."
+ start = Time.now
+ pids = supported.map do |vm|
+ # fork off a different process for each VM we want to test under
+ fork do
+ result = `rvm #{vm} do rake 2>/dev/null`
+
+ if result !~ /^Started$/
+ puts "** Couldn't run \"test rake\" under #{vm}!"
+ elsif result =~ /Command failed with status/
+ puts "** \"test rake\" failed under #{vm}!"
+ puts " (saving results to #{vm}-test-output.txt)"
+ File.open("#{vm}-test-output.txt", "w") { |f| f.write(result) }
+ else
+ failures = result[/(\d+) failures/, 1]
+ errors = result[/(\d+) errors/, 1]
+ if failures.nil? || errors.nil?
+ puts "** Couldn't parse test results under #{vm}"
+ puts " (saving results to #{vm}-test-output.txt)"
+ File.open("#{vm}-test-output.txt", "w") { |f| f.write(result) }
+ elsif failures == "0" && errors == "0"
+ puts "** Passed under #{vm}"
+ else
+ puts "** FAILED under #{vm}, #{failures} failures, #{errors} errors"
+ puts " (saving results to #{vm}-test-output.txt)"
+ File.open("#{vm}-test-output.txt", "w") { |f| f.write(result) }
+ end
+ end
+ end
+ end
+
+ pids.each { |pid| Process.wait(pid) }
+ puts "Finished in #{Time.now - start} seconds"
+end
View
1 lib/prawn.rb
@@ -8,6 +8,7 @@ module Prawn #:nodoc:
VERSION = "1.0.0.rc1"
end
+require "prawn/utilities"
require "prawn/core"
require "prawn/text"
require "prawn/graphics"
View
75 lib/prawn/font/afm.rb
@@ -27,8 +27,8 @@ def self.metrics_path
@metrics_path ||= [
".", "/usr/lib/afm",
"/usr/local/lib/afm",
- "/usr/openwin/lib/fonts/afm/",
- Prawn::DATADIR+'/fonts/']
+ "/usr/openwin/lib/fonts/afm",
+ Prawn::DATADIR+'/fonts']
end
end
@@ -41,18 +41,20 @@ def initialize(document, name, options={}) #:nodoc:
super
- @@winansi ||= Prawn::Encoding::WinAnsi.new
-
- @attributes = {}
- @glyph_widths = {}
- @bounding_boxes = {}
- @kern_pairs = {}
+ @@winansi ||= Prawn::Encoding::WinAnsi.new # parse data/encodings/win_ansi.txt once only
+ @@font_data ||= SynchronizedCache.new # parse each ATM font file once only
file_name = @name.dup
file_name << ".afm" unless file_name =~ /\.afm$/
file_name = file_name[0] == ?/ ? file_name : find_font(file_name)
- parse_afm(file_name)
+ font_data = @@font_data[file_name] ||= parse_afm(file_name)
+ @glyph_widths = font_data[:glyph_widths]
+ @glyph_table = font_data[:glyph_table]
+ @bounding_boxes = font_data[:bounding_boxes]
+ @kern_pairs = font_data[:kern_pairs]
+ @kern_pair_table = font_data[:kern_pair_table]
+ @attributes = font_data[:attributes]
@ascender = @attributes["ascender"].to_i
@descender = @attributes["descender"].to_i
@@ -152,6 +154,7 @@ def find_font(file)
end
def parse_afm(file_name)
+ data = {:glyph_widths => {}, :bounding_boxes => {}, :kern_pairs => {}, :attributes => {}}
section = []
File.foreach(file_name) do |line|
@@ -168,27 +171,41 @@ def parse_afm(file_name)
when ["FontMetrics", "CharMetrics"]
next unless line =~ /^CH?\s/
- name = line[/\bN\s+(\.?\w+)\s*;/, 1]
- @glyph_widths[name] = line[/\bWX\s+(\d+)\s*;/, 1].to_i
- @bounding_boxes[name] = line[/\bB\s+([^;]+);/, 1].to_s.rstrip
+ name = line[/\bN\s+(\.?\w+)\s*;/, 1]
+ data[:glyph_widths][name] = line[/\bWX\s+(\d+)\s*;/, 1].to_i
+ data[:bounding_boxes][name] = line[/\bB\s+([^;]+);/, 1].to_s.rstrip
when ["FontMetrics", "KernData", "KernPairs"]
next unless line =~ /^KPX\s+(\.?\w+)\s+(\.?\w+)\s+(-?\d+)/
- @kern_pairs[[$1, $2]] = $3.to_i
+ data[:kern_pairs][[$1, $2]] = $3.to_i
when ["FontMetrics", "KernData", "TrackKern"],
["FontMetrics", "Composites"]
next
else
- parse_generic_afm_attribute(line)
+ parse_generic_afm_attribute(line, data)
end
end
+
+ # process data parsed from AFM file to build tables which
+ # will be used when measuring and kerning text
+ data[:glyph_table] = (0..255).map do |i|
+ data[:glyph_widths][Encoding::WinAnsi::CHARACTERS[i]].to_i
+ end
+
+ character_hash = Hash[Encoding::WinAnsi::CHARACTERS.zip((0..Encoding::WinAnsi::CHARACTERS.size).to_a)]
+ data[:kern_pair_table] = data[:kern_pairs].inject({}) do |h,p|
+ h[p[0].map { |n| character_hash[n] }] = p[1]
+ h
+ end
+
+ data.each_value { |hash| hash.freeze }
+ data.freeze
end
- def parse_generic_afm_attribute(line)
+ def parse_generic_afm_attribute(line, hash)
line =~ /(^\w+)\s+(.*)/
key, value = $1.to_s.downcase, $2
- @attributes[key] = @attributes[key] ?
- Array(@attributes[key]) << value : value
+ hash[:attributes][key] = hash[:attributes][key] ? Array(hash[:attributes][key]) << value : value
end
# converts a string into an array with spacing offsets
@@ -200,10 +217,8 @@ def kern(string)
kerned = [[]]
last_byte = nil
- kern_pairs = latin_kern_pairs_table
-
string.bytes do |byte|
- if k = last_byte && kern_pairs[[last_byte, byte]]
+ if k = last_byte && @kern_pair_table[[last_byte, byte]]
kerned << -k << [byte]
else
kerned.last << byte
@@ -216,30 +231,12 @@ def kern(string)
e.respond_to?(:force_encoding) ? e.force_encoding("Windows-1252") : e
}
end
-
- def latin_kern_pairs_table
- return @kern_pairs_table if defined?(@kern_pairs_table)
-
- character_hash = Hash[Encoding::WinAnsi::CHARACTERS.zip((0..Encoding::WinAnsi::CHARACTERS.size).to_a)]
- @kern_pairs_table = @kern_pairs.inject({}) do |h,p|
- h[p[0].map { |n| character_hash[n] }] = p[1]
- h
- end
- end
-
- def latin_glyphs_table
- @glyphs_table ||= (0..255).map do |i|
- @glyph_widths[Encoding::WinAnsi::CHARACTERS[i]].to_i
- end
- end
private
def unscaled_width_of(string)
- glyph_table = latin_glyphs_table
-
string.bytes.inject(0) do |s,r|
- s + glyph_table[r]
+ s + @glyph_table[r]
end
end
end
View
16 lib/prawn/images/png.rb
@@ -262,7 +262,7 @@ def min_pdf_version
private
def unfilter_image_data
- data = Zlib::Inflate.inflate(@img_data).unpack 'C*'
+ data = Zlib::Inflate.inflate(@img_data).bytes
@img_data = ""
@alpha_channel = ""
@@ -270,9 +270,16 @@ def unfilter_image_data
scanline_length = pixel_bytes * self.width + 1
row = 0
pixels = []
+ row_data = [] # reused for each row of the image
paeth, pa, pb, pc = nil
- until data.empty? do
- row_data = data.slice! 0, scanline_length
+
+ data.each do |byte|
+ # accumulate a whole scanline of bytes, and then process it all at once
+ # we could do this with Enumerable#each_slice, but it allocates memory,
+ # and we are trying to avoid that
+ row_data << byte
+ next if row_data.length < scanline_length
+
filter = row_data.shift
case filter
when 0 # None
@@ -335,9 +342,10 @@ def unfilter_image_data
end
pixels << s
row += 1
+ row_data.clear
end
- # convert the pixel data to seperate strings for colours and alpha
+ # convert the pixel data to separate strings for colours and alpha
color_byte_size = self.colors * self.bits / 8
alpha_byte_size = alpha_channel_bits / 8
pixels.each do |this_row|
View
44 lib/prawn/utilities.rb
@@ -0,0 +1,44 @@
+# encoding: utf-8
+
+# utilities.rb : General-purpose utility classes which don't fit anywhere else
+#
+# Copyright August 2012, Alex Dowad. All Rights Reserved.
+#
+# This is free software. Please see the LICENSE and COPYING files for details.
+
+require 'thread'
+
+module Prawn
+
+ # Throughout the Prawn codebase, repeated calculations which can benefit from caching are made
+ # In some cases, caching and reusing results can not only save CPU cycles but also greatly
+ # reduce memory requirements
+ # But at the same time, we don't want to throw away thread safety
+ # We have two interchangeable thread-safe cache implementations:
+
+ class SynchronizedCache
+ # As an optimization, this could access the hash directly on VMs with a global interpreter lock (like MRI)
+ def initialize
+ @cache = {}
+ @mutex = Mutex.new
+ end
+ def [](key)
+ @mutex.synchronize { @cache[key] }
+ end
+ def []=(key,value)
+ @mutex.synchronize { @cache[key] = value }
+ end
+ end
+
+ class ThreadLocalCache
+ def initialize
+ @cache_id = "cache_#{self.object_id}".to_sym
+ end
+ def [](key)
+ (Thread.current[@cache_id] ||= {})[key]
+ end
+ def []=(key,value)
+ (Thread.current[@cache_id] ||= {})[key] = value
+ end
+ end
+end
View
7 spec/png_spec.rb
@@ -188,14 +188,17 @@
png = Prawn::Images::PNG.new(@img_data)
png.split_alpha_channel!
data = File.binread(@data_filename)
- png.img_data.should == data
+ # compare decompressed rather than compressed image data
+ # because JRuby's implementation of Zlib is different from MRI --
+ # both generate valid gzipped data, but not bit-identical to each other
+ Zlib::Inflate.inflate(png.img_data).should == Zlib::Inflate.inflate(data)
end
it "should correctly extract the alpha channel data from the image data chunk" do
png = Prawn::Images::PNG.new(@img_data)
png.split_alpha_channel!
data = File.binread(@alpha_data_filename)
- png.alpha_channel.should == data
+ Zlib::Inflate.inflate(png.alpha_channel).should == Zlib::Inflate.inflate(data)
end
end
Something went wrong with that request. Please try again.