Multiple warnings #1184
Comments
|
The uninitialized instance variable warnings are expected, Sequel purposely does not initialize instance variables for performance reasons. I may consider fixing other warnings, though the example you gave includes only uninitialized instance variable warnings. I've proposed a ruby API for filtering warnings (https://bugs.ruby-lang.org/issues/12299), which would allow Sequel to be less noisy in verbose mode. It should be discussed at the ruby developers meeting next week. Hopefully matz will accept my proposal or an approach that offers comparable functionality. |
|
I understand your proposal for selective warnings, however, not initializing variables due to performance, raises some doubt in my head. How much performance exactly are we gaining? If we benchmark a real world application with vanilla Sequel, and a patched Sequel (to initialize all variables), how much performance do we loose (%)? In general, other projects consider warnings to be "not good", and usually accept patches to suppress them. I imagine this performance argument must be very heavy to consider it different in Sequel. In the meantime, I used the code below in my project, to suppress warnings around every Sequel call (which is annoying). Might be useful to other users. The first one suppress warnings in code inside given block, the second one, can be used to convert an Enumerator to another Enumerator with suppressed warnings (useful for data sets). # Execute given block with <tt>$VERBOSE = false</tt>.
def no_verbose
original_verbose = $VERBOSE
begin
$VERBOSE = false
yield
ensure
$VERBOSE = original_verbose
end
end
# Receives an Enumerable from given block, and returns a new Enumerable, that have with <tt>$VERBOSE = false</tt> when executing. Ex:
# no_verbose_enum{[1,2,3].to_enum}.each{|e| puts e}
def no_verbose_enum
enum = no_verbose { yield }
Enumerator.new do |yilder|
loop do
element = nil
begin
no_verbose { element = enum.next }
rescue StopIteration
break
else
yilder << element
end
end
end
end |
|
The performance overhead of initializing instance variables to nil is significant, both in the time it takes to do the initialization and the extra memory it uses. See https://bugs.ruby-lang.org/issues/10396 for a benchmark. |
|
Also, if you want to want to see what's involved in patching Sequel to remove verbose warnings, I had a diff to do so (now a couple years old): https://groups.google.com/forum/#!searchin/sequel-talk/warning/sequel-talk/9aehjtVM6II/6BpInBub1LcJ |
|
OK, I understand there is a pontual performance impact, I am not doubting that at all. I meant how much does it affect overall performance in a real world application. Is this variable initialization any bottleneck? Does it add 15% more CPU time for any request made? To fully form an opinion, I'd have to remake the no-warnings patch, run some (close to) real world benchmarks over Sequel within an application, and plot the results. I can do that, no problem, but only if it would be relevant to changing the project direction over warnings. I am guessing performance impact overall is gonna be tiny, and that the value of having warnings enabled would be greater than suppressing them in such case, as warnings can help spot programming mistakes. |
|
Initializing instance variables to nil had a measureable difference when retrieving large datasets when I last benchmarked it. I forget the exact numbers, but I think between 5-15%, depending on which model plugins were in use. Anything above 0.1% I would consider unacceptable from a performance perspective. Additionally, initializing instance variables to nil clutters the code base. The value of uninitialized instance variable warnings in Sequel is tiny, I am not sure there has every been a bug in Sequel due to a misnamed instance variable. I initially proposed from removing the uninitialized instance variable warning from ruby completely, which would provide a substantial speedup by itself. Alas, that was not accepted, so I'm working on making it easy to filter warnings, so that when using Sequel in future versions of ruby, uninitialized instance variable warnings in Sequel will not be emitted, but you can still have uninitialized instance variable warnings in your application code. |
|
5-15% is really high! That being, fixing the warnings would solve one problem, but create other, tough choice. I do get your point. |
|
The performance issue is a fair one, but if you're willing to make the trade-off for your test environment, I ended up doing this: # spec/support/sequel.rb
SEQUEL_GEM_PATH = Gem.loaded_specs['sequel'].full_gem_path
Warning.class_eval do
alias original_warn warn
def warn(message)
original_warn(message) unless message.match?(SEQUEL_GEM_PATH)
end
end |
|
@Aupajo and Jeremy created a gem for automating this https://github.com/jeremyevans/ruby-warning :) |
Complete Description of Issue
Sequel gives a lot of warnings while running. I usually execute my tests with warnings enabled, and Sequel is giving a lot of unneeded output. They seem harmless, but surely are noisy.
Simplest Possible Self-Contained Example Showing the Bug
The text was updated successfully, but these errors were encountered: