Add workaround to handle zero sized files in /proc #3411

Val · 2016-10-11T18:04:57Z

RX14 · 2016-10-11T18:40:31Z

src/file.cr

@@ -417,6 +417,11 @@ class File < IO::FileDescriptor
  # ```
  def self.read(filename, encoding = nil, invalid = nil) : String
    File.open(filename, "r") do |file|
+      {% if flag?(:linux) %}
+      # workaround to handle zero sized files in /proc Linux virtual filesystem
+      encoding ||= "UTF-8" if File.expand_path(filename) =~ /^\/proc/


Why not use starts_with?? Regexes are quite slow and unnecessary for this.

Maybe it would be more clear to add a specific boolean variable that makes it enter the gets_to_end branch instead of overloading encoding?

refi64 · 2016-10-11T18:45:54Z

spec/std/file_spec.cr

@@ -37,6 +37,13 @@ describe "File" do
    str.should eq("Hello World\n" * 20)
  end

+  {% if !flag?(:windows) %}


This should be {% if flag?(:linux) %}; this doesn't work on OSX.

Yeah, /proc is only pretty much guaranteed to be mounted on windows, but it can be mounted on most unixes.

Fix crystal-lang#3411

asterite · 2016-10-11T19:28:54Z

I would fix this by changing File.read to open the file and invoke gets_to_end when the file size is zero, or always. The problem has nothing to do with encoding.

Fix crystal-lang#3410

RX14 · 2016-10-11T19:46:02Z

src/file.cr

@@ -417,6 +417,10 @@ class File < IO::FileDescriptor
  # ```
  def self.read(filename, encoding = nil, invalid = nil) : String
    File.open(filename, "r") do |file|
+      {% if flag?(:linux) %}
+      return file.gets_to_end if File.expand_path(filename).starts_with?("/proc")


This now ignores encoding for /proc*.

asterite · 2016-10-11T20:11:03Z

@Val we finally decided with @waj that the safest thing to do, even if it's a bit slower, is to always invoke gets_to_end. So basically it should be:

  def self.read(filename, encoding = nil, invalid = nil) : String
    File.open(filename, "r") do |file|
      file.set_encoding(encoding, invalid: invalid) if encoding
      file.gets_to_end
    end
  end

But I'll let you change this in your PR because you have the specs, and you found the bug :-)

RX14 · 2016-10-11T20:58:20Z

@asterite I think File.read is a common enough call that optimising it is worthwhile.
I think we should at least do some benchmarks to determine the slowdown factor before we merge this.

Val · 2016-10-11T21:17:55Z

path = "/proc/self/cmdline"

def read1(filename, encoding = nil, invalid = nil) : String
  File.open(filename, "r") do |file|
    if encoding
      file.set_encoding(encoding, invalid: invalid)
      file.gets_to_end
    else
      {% if flag?(:linux) %}
      return file.gets_to_end if File.expand_path(filename).starts_with?("/proc")
      {% end %}

      size = file.size.to_i
      String.new(size) do |buffer|
        file.read Slice.new(buffer, size)
        {size.to_i, 0}
      end
    end
  end
end

def read2(filename, encoding = nil, invalid = nil) : String
  File.open(filename, "r") do |file|
    file.set_encoding(encoding, invalid: invalid) if encoding
    file.gets_to_end
  end
end

require "benchmark"
Benchmark.ips do |x|
  x.report("read1") { read1 path }
  x.report("read2") { read2 path }
end

On a Debian jessie amd64:

>  crystal build --release file_read_bench.rb
> ./file_read_bench
read1 174.99k (± 6.28%)  1.73× slower
read2 301.93k (± 5.34%)       fastest
> ./file_read_bench
read1 212.89k (± 5.30%)  1.24× slower
read2 263.87k (± 3.41%)       fastest

RX14 · 2016-10-11T21:23:18Z

@Val can you also test on various sizes of normal file, say 1kb, 1mb,10mb?

Val · 2016-10-11T21:52:54Z

path = "/tmp/#{File.basename(__FILE__)}"

def read1(filename, encoding = nil, invalid = nil) : String
  File.open(filename, "r") do |file|
    if encoding
      file.set_encoding(encoding, invalid: invalid)
      file.gets_to_end
    else
      {% if flag?(:linux) %}
      return file.gets_to_end if File.expand_path(filename).starts_with?("/proc")
      {% end %}

      size = file.size.to_i
      String.new(size) do |buffer|
        file.read Slice.new(buffer, size)
        {size.to_i, 0}
      end
    end
  end
end

def read2(filename, encoding = nil, invalid = nil) : String
  File.open(filename, "r") do |file|
    file.set_encoding(encoding, invalid: invalid) if encoding
    file.gets_to_end
  end
end

BLOBS = {} of Symbol => String
kb_01 = BLOBS[:kb_01] = "X" * 1024
mb_01 = BLOBS[:mb_01] = kb_01 * 1024
BLOBS[:mb_10] = mb_01 * 10

require "benchmark"

BLOBS.each do |name, blob|
  File.open(path, "w") { |f| f.print blob }

  puts "#{name} _____________________________________\n"
  Benchmark.ips do |x|
    x.report("read1 #{name}") { read1 path }
    x.report("read2 #{name}") { read2 path }
  end
  puts
end

File.delete path

Output

> crystal build --release file_read_bench.cr 
> ./file_read_bench 
kb_01 _____________________________________

read1 kb_01 349.94k (± 2.17%)       fastest
read2 kb_01 336.11k (± 2.58%)  1.04× slower

mb_01 _____________________________________

read1 mb_01   9.43k (± 2.27%)       fastest
read2 mb_01   2.32k (± 2.69%)  4.07× slower

mb_10 _____________________________________

read1 mb_10  532.3  (± 3.12%)       fastest
read2 mb_10 157.96  (± 1.78%)  3.37× slower

Val · 2016-10-11T21:56:32Z

Subtle, changing return file.gets_to_end if File.expand_path(filename).starts_with?("/tmp").

> ./file_read_bench 
kb_01 _____________________________________

read1 kb_01 298.51k (± 2.54%)  1.13× slower
read2 kb_01 337.64k (± 3.03%)       fastest

mb_01 _____________________________________

read1 mb_01   2.32k (± 4.93%)       fastest
read2 mb_01   2.31k (± 6.39%)  1.01× slower

mb_10 _____________________________________

read1 mb_10 154.89  (± 5.72%)  1.04× slower
read2 mb_10 161.59  (± 4.46%)       fastest

Val · 2016-10-11T22:55:50Z

@RX14 & @asterite it seems that 416b0e7 should be reverted, what do you think?

Add workaround to handle zero sized files in /proc

81c8bc7

Fix crystal-lang#3410

RX14 requested changes Oct 11, 2016

View reviewed changes

refi64 reviewed Oct 11, 2016

View reviewed changes

Limit /proc File#read spec to Linux OS only, thx @kirbyfan64

b3c826c

Fix crystal-lang#3411

Val force-pushed the proposed_fix_for_#3410 branch from 8479c45 to b3c826c Compare October 11, 2016 19:38

Do not use encoding for /proc linux file handling, thx @RX14 & @asterite

3f9d794

Fix crystal-lang#3410

RX14 reviewed Oct 11, 2016

View reviewed changes

Do not avoid encoding, thx @RX14

9fa32dc

Always invoke gets_to_end, thx @asterite & @waj.

416b0e7

asterite closed this in a33c0c2 Oct 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workaround to handle zero sized files in /proc #3411

Add workaround to handle zero sized files in /proc #3411

Val commented Oct 11, 2016

RX14 Oct 11, 2016

RX14 Oct 11, 2016

refi64 Oct 11, 2016

RX14 Oct 11, 2016

asterite commented Oct 11, 2016

RX14 Oct 11, 2016

asterite commented Oct 11, 2016

RX14 commented Oct 11, 2016

Val commented Oct 11, 2016 •

edited

Loading

RX14 commented Oct 11, 2016

Val commented Oct 11, 2016

Val commented Oct 11, 2016

Val commented Oct 11, 2016

Add workaround to handle zero sized files in /proc #3411

Add workaround to handle zero sized files in /proc #3411

Conversation

Val commented Oct 11, 2016

RX14 Oct 11, 2016

Choose a reason for hiding this comment

RX14 Oct 11, 2016

Choose a reason for hiding this comment

refi64 Oct 11, 2016

Choose a reason for hiding this comment

RX14 Oct 11, 2016

Choose a reason for hiding this comment

asterite commented Oct 11, 2016

RX14 Oct 11, 2016

Choose a reason for hiding this comment

asterite commented Oct 11, 2016

RX14 commented Oct 11, 2016

Val commented Oct 11, 2016 • edited Loading

RX14 commented Oct 11, 2016

Val commented Oct 11, 2016

Val commented Oct 11, 2016

Val commented Oct 11, 2016

Val commented Oct 11, 2016 •

edited

Loading