Performance issues? #38
Comments
|
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 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? |
|
Using the progress bar with advance and then using strace -e openat |
|
Ahh, on closer look I know where these calls are coming from! This gem uses 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? |
|
Since these things are process global, you should probably do it at load time. |
|
You can also do things like |
|
That's pretty much what happens if you look at the link I provided. However, the |
Is trying to require fiddle every time the method is called. |
|
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. |
|
So would you move all the requires up the top of the file and try to load each one of them wrapped in? 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? |
|
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! 🙌 |
|
I guess from my POV, I do prefer something at the class scope like: |
|
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
endAnd 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 |
|
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. |
|
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! |
|
Fixed in |
|
Thanks for your work here! Much appreciated :) |
|
Thanks for reporting and I mean it! This helped me make the |
This seems super weird, but when I use
progress.advance(1), I get a bunch of syscalls:Do you have any
requirestatement embedded in code at some point? Removing the progress bar = no more syscalls of this nature.The text was updated successfully, but these errors were encountered: