inspect memory limit fix by viuginick1 · Pull Request #100 · ruby-debug/ruby-debug-ide

viuginick1 · 2017-07-01T01:26:20Z

Sets a memory limit for execution of inspect (default 10mb)

valich

Some (mostly) code style problems must be addressed before merging

valich · 2017-07-04T14:16:09Z

bin/rdebug-ide

    options.evaluation_timeout = timeout
  end
  opts.on('--stop', 'stop when the script is loaded') {options.stop = true}
+


Is this change needed here?

valich · 2017-07-04T14:16:17Z

bin/rdebug-ide

  end
 else
  Debugger.debug_program(options)
-end


valich · 2017-07-04T14:17:45Z

lib/ruby-debug-ide/xml_printer.rb

    end

+    def inspect_with_allocation_control(slice, memory_limit)
+      x = Thread.current


Can we conceive a better name for this variable? Like current_thread, start_thread or alike

valich · 2017-07-04T14:18:15Z

lib/ruby-debug-ide/xml_printer.rb

+      trace = TracePoint.new(:c_call, :call) do |tp|
+        curr_alloc_size = ObjectSpace.memsize_of_all
+
+        if(curr_alloc_size - start_alloc_size > 1e6*memory_limit)


valich · 2017-07-04T14:21:26Z

lib/ruby-debug-ide/xml_printer.rb

+        end
+      end
+
+      trace.enable


I think it's better to use TracePoint#enable( &block) here

TracePoint#enable( &block) works differently

Could you please elaborate more?

If a block is given, the trace will only be enabled within the scope of the block.

In my implementation, the checks are carried out more often, which means that the probability of detecting an overflow increases

Yes, so I suggest changing these three lines with

trace.enable { slice.inspect }

In my implementation, the checks are carried out more often, which means that the probability of detecting an overflow increases

Sorry, did not get what overflow you're talking about.

valich · 2017-07-04T14:22:21Z

lib/ruby-debug-ide/xml_printer.rb

+      slice.inspect
+      trace.disable 
+    rescue MemoryLimitError
+      return nil


Do you think it makes sense to add some logging for such cases? Or return something very special

Added logging

I think printing also a stacktrace of an exception may help investigating the problems in inspect calls for users.

This backtrack will end with the calls of our debase and r-d-i methods. I think that this is not very useful.

valich · 2017-07-04T14:31:29Z

lib/ruby-debug-ide/xml_printer.rb

      slice   = value[0..10]
-      compact = slice.inspect
+
+      if (defined?(JRUBY_VERSION) || ENV['DEBUGGER_MEMORY_LIMIT'].to_i <= 0)


this could be replaced with compact = if (...)

I do not think that this is applicable in this case

Why so? Please see https://github.com/bbatsov/ruby-style-guide#use-if-case-returns

valich · 2017-07-04T14:31:40Z

lib/ruby-debug-ide/xml_printer.rb

  end

-end
+end


valich · 2017-07-05T13:27:34Z

lib/ruby-debug-ide/command.rb

      begin
        str = str.to_s
-        str.force_encoding('UTF-8') if(RUBY_VERSION > 2.0)
+        str.force_encoding('UTF-8') if(RUBY_VERSION > '2.0')


does this relate to the logging changes?

valich · 2017-07-05T13:28:00Z

lib/ruby-debug-ide/xml_printer.rb

    end

    def print_msg(*args)
+      puts 'def print_msg(*args)'


Is this puts needed?

valich · 2017-07-05T13:30:28Z

lib/ruby-debug-ide/xml_printer.rb

                  slice.inspect
                else  
-                  compact = inspect_with_allocation_control(slice, ENV['DEBUGGER_MEMORY_LIMIT'].to_i)
+                  compact = inspect_with_allocation_control(slice, ENV['DEBUGGER_MEMORY_LIMIT'].to_i, value.object_id.to_s)


I am not sure object_id us really useful for logging purposes: it does not tell anything about the object. I think the previous message already a good start.

valich · 2017-07-05T13:33:35Z

lib/ruby-debug-ide/xml_printer.rb

      trace.disable
      result 
    rescue MemoryLimitError => e
+      print_msg(e.message)


I think one may make use of --debug option here:

opts.on("-d", "--debug", "Debug self - prints information for debugging ruby-debug itself") do Debugger.cli_debug = true options.cli_debug = true end

valich · 2017-07-05T18:42:21Z

bin/rdebug-ide


-  ENV['DEBUGGER_MEMORY_LIMIT'] = '10'
-  opts.on("-m", "--memory-limit LIMIT", Integer, "evaluation memory limit in mb (default: 10)") do |limit| 
+  ENV['DEBUGGER_MEMORY_LIMIT'] = '1'


What stands behind such defaults change? Could you please reason it?

With 10 you have to wait for a pretty long time

valich · 2017-07-05T18:43:34Z

lib/ruby-debug-ide/xml_printer.rb

-          curr_thread.raise MemoryLimitError, "Out of memory: evaluation of inspect took more than #{memory_limit}mb." if curr_thread.alive?
-        end
+      if Thread.respond_to?(:critical) and Thread.critical
+        raise ThreadError, "memory_limit within critical session"      


AFAIU the execution of that line does not mean memory limit is exceeded. What does this check?

valich · 2017-07-05T18:44:36Z

lib/ruby-debug-ide/xml_printer.rb

+        start_alloc_size = ObjectSpace.memsize_of_all
+        trace = TracePoint.new(:c_call, :call) do |tp|
+          curr_alloc_size = ObjectSpace.memsize_of_all
+          start_alloc_size = curr_alloc_size if (curr_alloc_size < start_alloc_size)


The case of GC?

valich · 2017-07-05T18:46:14Z

lib/ruby-debug-ide/xml_printer.rb

+      result = nil
+      inspect_thread = DebugThread.start {
+        start_alloc_size = ObjectSpace.memsize_of_all
+        trace = TracePoint.new(:c_call, :call) do |tp|


Do you need trace variable? I don't see any usages..

I'll disable it through 'trace'

valich · 2017-07-05T18:46:58Z

lib/ruby-debug-ide/xml_printer.rb

+
+          if(curr_alloc_size - start_alloc_size > 1e6 * memory_limit)
+            curr_thread.raise MemoryLimitError, "Out of memory: evaluation of inspect took more than #{memory_limit}mb. \n#{caller.map{|l| "\t#{l}"}.join("\n")}"
+            inspect_thread.kill


I'd rather move this kill to line 410, after join. It seems strange to me to kill the thread from this thread itself.

valich · 2017-07-07T13:41:16Z

lib/ruby-debug-ide/xml_printer.rb

@@ -389,9 +392,19 @@ def inspect_with_allocation_control(slice, memory_limit)
      result = nil
      inspect_thread = DebugThread.start {
        start_alloc_size = ObjectSpace.memsize_of_all


Overall looks nice. One last question: do you think such checks on every call will slow down the execution?
The possible solution there is to make these checks only in 10% of calls (like Math.random > 0.9) or alike

Most often 'inspect' works fast and requires little memory, so in general the performance should not slow down. And in case of heavy inspect-s, when using probability, everything can crush with a high probability. Then why was this fix at all?)

Perhaps, it is worth running a :call and :c_call handler a little less often. Every fifth, for example.

Perhaps, it is worth running a :call and :c_call handler a little less often. Every fifth, for example.

That's what I'm talking about.

inspect often works fast, but

We add constant overhead on each call which may slow execution down in dozens of times;

There are many inspects during variables collection so in total this matters

I still getting an INTERNAL ERROR (pipe or something) when using probabilistic scheme. Apparently, it is better to abandon this proposal.
https://pastebin.com/SL8R1DGc

Can you show the snippet you're using to achieve the result?

valich · 2017-07-11T08:33:07Z

lib/ruby-debug-ide/xml_printer.rb

+      inspect_thread = DebugThread.start {
+        start_alloc_size = ObjectSpace.memsize_of_all
+        start_time = Time.now
+        max_time = Debugger.evaluation_timeout


I think 10s evaluation timeout is too much for this place. The reason is that evaluation is an explicit user action so it's OK to try to wait for a possibly long evaluation. In the case of convenience infos calculated dozens of times per breakpoint, 10s does not make much sense to me. I'd say we should make it somewhere between 100 and 500ms.

…ariable inspect

valich

Minor changes still needed

valich · 2017-07-24T21:27:49Z

lib/ruby-debug-ide/xml_printer.rb

    end
-
+
+    def exec_with_allocation_control(value, memory_limit, time_limit, exec_method, flag)


what does flag mean? Not a helpful name for a parameter

valich · 2017-07-24T21:28:23Z

lib/ruby-debug-ide/xml_printer.rb

+    attr_reader :message
+    attr_reader :backtrace
+
+    def initialize(message, backtrace)


This one does not provide default backtrace value and the one below does. Is that intentional?

valich · 2017-07-24T21:28:34Z

lib/ruby-debug-ide/xml_printer.rb

+    rescue MemoryLimitError, TimeLimitError => e
+      print_debug(e.message + "\n" + e.backtrace)
+
+      return nil if flag


valich · 2017-07-25T12:23:05Z

Squashed and merged into master.

viuginick1 added 4 commits July 1, 2017 04:21

inspect memory limit

d591919

cleanup

045a84c

Option to disable memory check is added

5d0ac96

disable memory limit in the same option

9665253

valich self-assigned this Jul 4, 2017

valich self-requested a review July 4, 2017 14:11

valich requested changes Jul 4, 2017

View reviewed changes

viuginick1 force-pushed the upstream branch 2 times, most recently from a6dff63 to 9665253 Compare July 5, 2017 12:09

viuginick1 added 3 commits July 5, 2017 15:13

cleanup: formating fix, rename variable, rewrite on "compact = if (...)"

740c656

fixed exeption message

e5d7d57

memory limit logging added

9a9c981

viuginick1 force-pushed the upstream branch from 0dba1ef to 9a9c981 Compare July 5, 2017 13:29

valich requested changes Jul 5, 2017

View reviewed changes

viuginick1 added 3 commits July 5, 2017 16:44

debug output deleted

cc45f3b

print_msg -> print_debug

b15847f

object_id deleted from the exeption message

8328947

valich requested changes Jul 5, 2017

View reviewed changes

Fixes according to the review

7b3a875

viuginick1 force-pushed the upstream branch from 8fd8ad5 to 7b3a875 Compare July 6, 2017 11:16

timelimit for inspect evaluation added

84f17fd

valich reviewed Jul 7, 2017

View reviewed changes

viuginick1 added 2 commits July 7, 2017 19:15

The handler is called with a probability of 25%

f7f64a9

cleanup

57940ab

valich approved these changes Jul 11, 2017

View reviewed changes

valich requested changes Jul 11, 2017

View reviewed changes

inspect_with_alloc_control -> exec_with_... and heuristic break for v…

bc8e07c

…ariable inspect

valich requested changes Jul 24, 2017

View reviewed changes

fixes according to reiew

0b80618

valich approved these changes Jul 25, 2017

View reviewed changes

valich merged commit 03a6101 into ruby-debug:master Jul 25, 2017

                 end
-              end
+              end

                
                    No newline at end of file

		end


		def exec_with_allocation_control(value, memory_limit, time_limit, exec_method, flag)

Conversation

viuginick1 commented Jul 1, 2017

Uh oh!

valich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viuginick1 Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viuginick1 Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viuginick1 Jul 5, 2017 •

edited

Loading

viuginick1 Jul 6, 2017 •

edited

Loading

viuginick1 Jul 7, 2017 •

edited

Loading