Performance issues? about tty-progressbar HOT 17 CLOSED

Thanks for reporting and I mean it! This helped me make the tty-screen gem much better.

from tty-progressbar.

https://github.com/piotrmurach/tty-screen/blob/c76fbbc9047baa8624a71ec589e60c358efacc4c/lib/tty/screen.rb#L39-L50

You shouldn't be doing it like this, you should probably cache which platform you are on and only invoke the right function. Otherwise every call to size is potentially doing requires/searches.

from tty-progressbar.

So would you move all the requires up the top of the file and try to load each one of them wrapped in?

begin
  ...
rescue LoadError
end

As for the platform, apart from the windows api call, all the other methods are platform-independent. So not sure caching would help much here. The searches/checks are necessary since the terminal size can change and you want to have a 'fresh' value every time. It's probably down to use whether they want to 'cache' terminal size or keep reading the current value?

from tty-progressbar.

I agree that probably a good way would be to find a method during the initial check that returns a non-zero size and then cache the actual method name of the call for future checks. I will add some performance tests and refactor. Thanks for bringing this up! 🙌

from tty-progressbar.

Thanks for your work here! Much appreciated :)

from tty-progressbar.

Hi Sam,

That's some weird issue! Never see it before. I wonder if that's something to do with Ruby 2.7. Do you get the same error, for example, on Ruby 2.6? I wonder if io/console has been removed from Ruby and moved to a separate gem and that's what's causing the issue. It's the most likely thing to change between the Ruby releases.

There are no embedded requires but this gems has couple dependencies. The main code is a single file and all the requires are listed in the file header.

Do you have a test case that replicates this?

from tty-progressbar.

Using the progress bar with advance and then using strace -e openat

from tty-progressbar.

Ahh, on closer look I know where these calls are coming from!

This gem uses tty-screen to detect terminal width and the very first size call check uses the windows api to get the console size. The size_from_win_api attempts to load fiddle module and kernel32 lib. On Unix this will silently fail and move on to another check.

This check only makes sense on windows. So I wonder if I should check first the platform and skip to checking different sources. Any thoughts?

from tty-progressbar.

Since these things are process global, you should probably do it at load time.

if windows
  require 'blah'
  def self.size
    blah
  end
end

from tty-progressbar.

You can also do things like

begin
  require 'fiddle'
rescue LoadError
  # it wasn't available.
end

from tty-progressbar.

That's pretty much what happens if you look at the link I provided. However, the fiddle module is loaded fine, it's the search for the kernel32 dll lib that fails on Unix-like system.

from tty-progressbar.

    def size_from_win_api(verbose: nil)
      require 'fiddle'

Is trying to require fiddle every time the method is called.

from tty-progressbar.

I guess from my POV, I do prefer something at the class scope like:

if RUBY_VERSION > 2.7
  def size
    # Fast implementation using io/console
  end
elsif RUBY_PLATFORM =~ Windows
  require 'fiddle'
  KERNEL = Fiddle.kernel32 or abort "Could not load kernel"

  def size
    KERNEL.gremlins
  end
else
  # ...
end

from tty-progressbar.

I'm kind of thinking of moving all the methods to separate files:

# lib/tty/size/windows_api
begin
  require 'fiddle'
rescue
end

module WindowsAPi
   def self.size
  end
end

# lib/tty/screen/io_console
module IOConsole
  def self.size
  end
end

And then require them all(load all the requires) and detect the method that reads size correctly:

require_relative "screen/windows_api"
require_relative "screen/io_console"

module Screen
  @size_cache = nil

  def self.size
    return @size_cache.call if @size_cache

     if WindowsAPI.size
       @size_cache = WindowsAPI.method(:size)
     elsif IOConsole.size
       @size_cache = IOConsole.method(:size)
     else
         ... 
     end
  end
end

from tty-progressbar.

I guess it depends on how much "engineering" you want to maintain and whether it is a big enough chunk of code to warrant that design. It's not bad... but I still think you are better just using class-level if statements.

from tty-progressbar.

Agree, I don't want to go overboard with the design. Actually, the main point is to stop the cascading of checks if a known method works - this will defo improve performance. I need to have a play and see. Leave it with me and I will let you know once a 'good enough' version exists. Again, thanks for your help!

from tty-progressbar.

Fixed in v0.8.0 release of the tty-screen gem which brings a lot of performance improvements.

from tty-progressbar.