Performance enhancement #155

Closed
wants to merge 3 commits into
from

Projects

None yet

4 participants

@mikefarah

No longer reads PDF into memory when outputing to file. This can save significant time for large pdfs.

Mike Farah Performance enhancement
No longer reads PDF into memory when outputing to file
b500bd6
@devn
pdfkit member

Thanks for the PR! Could you please add a test for this?

@mikefarah

As far as I can tell it should be covered by the existing tests, just runs faster when writing to a file

@devn
pdfkit member
@devn
pdfkit member

Hey Mike,

Okay, sure -- I'll buy that.

Could you please resubmit to clear up the merge conflict?

Thanks!

@mikefarah
Mike Farah added some commits Apr 20, 2013
Mike Farah Merge remote-tracking branch 'upstream/master'
Conflicts:
	lib/pdfkit/pdfkit.rb
58f349b
Mike Farah Added test for performance enhancement: 'No longer reads PDF into mem…
…ory when outputing to file'
768a0f8
@mikefarah

Alright I've fixed the merge conflict and added a test :)

@mikefarah

Any hints why the travis build is failing? I haven't been able to reproduce the issue

@devn
pdfkit member
@devn devn commented on the diff May 10, 2013
lib/pdfkit/pdfkit.rb
- # $? is thread safe per http://stackoverflow.com/questions/2164887/thread-safe-external-process-in-ruby-plus-checking-exitstatus
- raise "command failed: #{invoke}" if result.to_s.strip.empty? or !$?.success?
+ raise "command failed: #{invoke}" if (path && File.size(path) == 0) || (path.nil? && result.to_s.strip.empty?) || !$?.success?
@devn
devn May 10, 2013

Could you split this up into a couple of smaller predicates? This line is way too long and does not reveal intent.

@devn
pdfkit member

Sorry to keep stringing you along. This is not a huge issue, but I think this line can be cleaned up a bit and then I'm happy to merge.

@cdwort

@mikefarah The failing Travis build is discussed in #190. Solutions are welcome, of course! :)

Any interest in doing that final refactor? This seems so close.

Thanks!

@sigmavirus24
pdfkit member

@mikefarah I just merged this by cherry-picking the appropriate changes and updating the tests. Thanks for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment