Wow this one is pretty odd. It does not happen without the second file??? I really expected this to be something where the next somehow is missing hitting a matching push to our pop but it looks like it works ok if called from a lexical parent but not if it is not.

It doesn’t seem related to lexical scope, but to compiling. If I disable compilation, a second file isn’t needed:

$ cat foo.rb
while true
  begin
    raise "oops"
  rescue => e
    next
  end
end
$ jruby -J-Djruby.ir.debug -J-Djruby.compile.mode=OFF foo.rb 2>&1 | grep RPCs=1000
[main] INFO Interpreter : I: {15} toggle_backtrace(;true); <#RPCs=1000>
...

@andrewdotn ah yeah we compile main file by default. Thanks for that update.

I also see leaks with

while true
  while true
    begin
      raise "oops"
    rescue => e
      break
    end
  end
end

and

while true
  begin
    raise "oops"
  rescue => e
    redo
  end
end

I’ve been trying to make IRBuilder add an extra ExceptionRegionEndMarkerInstr before the jump in these cases, but while they stop the leaks on the simple examples here, it causes real applications to break later in CFG, and I haven’t dug into that yet.

This seems like a basic VM-level issue in our IR interpreter. The logic as compiled into JVM bytecode works just fine, because the management of try/catch regions is handled properly by the JVM. Any branch out of the "try" area automatically leaves that area, while for us it must be done manually at each exit point.

At first glance we create begin/end exc range which is great for building our CFG but then we try to leverage these as actual usable instrs in our startup interp, Their design is not really intended to pair up (e.g. start called then end called) as they are really for marking regions.

This is really only a problem in these cases that are infinite. It is also mildly ironic that we do not compile methods like this because they never exit so it is sort of an unhappy combination.

More notes. I tried pushing a new start exc only if it was different than top of stack (based on idea that lexically you could never raise to the same place twice) but this had issues with us prematurely popping too many elements. I cannot just ignore empty stack because I might have prematurely nuked a rescue ip we need.

@subbuss suggested another hack would be for start/end to both record the label they pair with and make sure to only pop if the push label matches.

Deleted my previous comment because I don't think that simplified logic is right ... but yes, this is a problem of retrofitting the startup interpreter on top of the IR meant for CFG construction. The hacky soln should work for now. Will let @enebo test it. If there is a cleaner non-hacky soln that we can think of, we can use it instead.

@andrewdotn since you were digging in last week I take it you can test this and give it the gold seal of approval :)

Looking at this problem in retrospect it seems obvious our solution was pretty lacking but we would only be able to observe this in an endless loop function which was not the main program file (which we compile by default). Even a huge finite loop I bet took a long time to OOM.

@andrewdotn Can you provide a test or spec that exercises this? Probably as part of either spec/compiler/general_spec.rb or something in test/jruby.

I don't think this is amenable to testing. Even with 32m heap it takes like 40 seconds to OOME

@enebo We can write a test that checks the stack size, though. Maybe it should be in Java.

@headius it is a local variable.

@enebo Ahh I didn't notice it was not part of the interpreter's instance state.

I tweaked the given script a bit to reduce allocation and eliminate backtrace generation, and it only takes about 5s to OOM with -Xmx32m.

args = [Exception, "foo", []]
while true
  begin
    raise *args
  rescue java::lang::OutOfMemoryError
    exit 1
  rescue Exception
    next
  end
end

It's still a bit clunky for a test.

Wow, thanks for the amazing turnaround time on this! I can confirm that your fix works. When I was debugging this, I added a check inside StartupInterpreterEngine.interpret() that would panic if rescuePCs grew larger than 100 elements. I think there could be a Java unit test along those lines, e.g., by checking that after 100 loops rescuePCs was a sane size. It would need rescuePCs to be exposed for testing.

If I was adding a test for my own code here, and I wasn’t worried about performance, I’d add a Consumer<Integer> rescuePCsListenerForTesting parameter, along with an overload that would keep the existing signature by defaulting the new parameter to null. If non-null, the consumer would be passed the size of rescuePCs on every push, so that the test could verify it didn’t grow excessively during the loop. I know that would be inelegant but it would be fast and it would work.

Ok so I found another issue but my confidence is quite a bit higher now. The reason I see no value in testing the stack size for unbounded growth is the algorithm on insertion will look to see if it is already on the stack and if so potentially reduce the size of the stack (but only grow if not present). So there should never be unbounded growth from that alone. However, we only bother to add these stack elements when we traverse a lexical section of code. I am super confident we will never be more than n lexical nestings of stack size.