<JasonLunn[m]> headius: Did you get an email from Google re: our Open Source Peer Bonus program?
<JasonLunn[m]> It may have arrive while you were on vacay, and / or may have looked like spam
<enebo[m]> yeah that static scope change was part of us removing use of IRScope for runtime lookup. This was significant changes leading up to startup work before the last FOSDEM talk we gave on startup time
<JasonLunn[m]> but it is actually real
<enebo[m]> It could be anything but IRScope and StaticScope in theory should contain the same values since they both get made very close together in time.
<headius> Jason Lunn: I did, thank you!
<headius> Is the move to FFI going forward at this point?
<JasonLunn[m]> The proof of concept implementation is complete,
<JasonLunn[m]> but there are some practical issues in getting it merged that I'd love to bounce off of you
<headius> Sure
<JasonLunn[m]> Better to chat here, or setup a dedicated time for a call?
<headius> In here works fine
<k77ch7[m]> enebo: headius hello! I try and measure impact. https://github.com/jruby/jruby/pull/7283#issuecomment-1206586746
<enebo[m]> k77ch7: Cool. yeah this could almost be noise so it is probably worth it to fix ability to interrupt
<enebo[m]> k77ch7: but we could possible only check every n back edges to then reduce the check to just a field read for most of the back edges too. This makes me think it may not be worth it though
<headius> There is a mechanism for that that increments a counter in the thread context that we certainly could use
<enebo[m]> Another detail headius also brought up yesterday is that ordinary ruby calls for methods written in Ruby will also perform an interupt check but in cases of simple math we optimize those calls and the interrupt check goes away. Just a detail woth pointing out
<enebo[m]> callThreadPoll.. headius I mention that in a comment on that issue but it is every 0xFFF invocations
<enebo[m]> which I suppose is not a huge number but it is an option for sure
<k77ch7[m]> I'm in Japanese time zone. so I will read your comments tomorrow. Good night.
<enebo[m]> k77ch7: good night. Nice to see you in matrix chat!
<k77ch7[m]> Me too.
<headius> yeah I agree the impact is probably nothing, and we can revisit this later on
<headius> merging!
subbu has joined #jruby
<headius> enebo: did you get a chance to look at the commit rsim mentions here:
<enebo[m]> > yeah that static scope change was part of us removing use of IRScope for runtime lookup. This was significant changes leading up to startup work before the last FOSDEM talk we gave on startup time
<enebo[m]> So not at why it would be different but this should be the same value...at least I would have thought so
<headius> Okay I'll have to dig deeper. Perhaps when setting up the StaticScope the jit is using a cached value
<enebo[m]> I am confused by the snippet
<enebo[m]> Is he saying FILE is being set manually or just saying what the value is
<headius> Just what the value is in both cases
<headius> But it seems like maybe the get_path logic is actually to blame now
<enebo[m]> It strikes me odd that moving to static scope would have any effect on this at all
<enebo[m]> where is get_path located?
<enebo[m]> RubyFile?
<enebo[m]> Only JIT hmm
<enebo[m]> headius: I will lay money down on Helpers#restoreScope
<enebo[m]> Although I am still confused by why this would be broken. It looks like an obvious place where the JIT changed by restoring file to StaticScope
<enebo[m]> I guess seeing --dev vs JIT and seeing what is in StaticScope.file to see if --dev stores fully qualified
<enebo[m]> To me I would expect it to the the short relative path but I guess I don't see any other reason it would be different
<headius> Yeah I keep forgetting the details. It can't just be that path method because the only effects jit. It's got to be getting the wrong path
<enebo[m]> newStaticScope also is a new signature in that commit too
<headius> ok yeah... it is a problem with how it deserializes
<headius> the original file name is stored in the dumped IR and that is used to recreate the static scope
<headius> LDC "blah.rb"
<headius> INVOKEVIRTUAL org/jruby/parser/StaticScope.setFile (Ljava/lang/String;)V
<enebo[m]> so in --dev staticscope stores fully qualified value?
<enebo[m]> I guess it must
<headius> it is populated at compile time based on actual path it was loaded from
<enebo[m]> oh
<enebo[m]> haha
<headius> but that compile-time value gets hardcoded into the jit/aot output
<enebo[m]> yeah I actually remember this but it has always been hardcoded to whenever it was compiled
<enebo[m]> I suppose the difference is it was getting it from IRScope which maybe saved it correctly?
<headius> we added the Filename operand for this, to get the runtime path for precompiled stuff
<headius> that operand is not used after your commit so it just stands up the StaticScope manually and inserts the hardcoded path
<enebo[m]> the impl of Filename is to ask StaticScope for File
<enebo[m]> or it is in the interp anyways
<headius> return context.runtime.newString(irScope == null ? currScope.getFile() : irScope.getFile());
<headius> return context.runtime.newString(file);
<enebo[m]> same in JIT ... I guess you at least know what is up
<headius> the first line is the old logic in getFileNameStringFromScope
<headius> this is what Filename operand used from jit code
<enebo[m]> they both used the same helper but obviously for interp it will have IRScope and that is ok
<enebo[m]> but in current interp the file set in currentscope is there and matches IRScope
<enebo[m]> so new impl is ok then
<headius> public IRScriptBody(IRManager manager, String sourceName, StaticScope staticScope) {
<headius> all the methods that get file from IRScope eventually bottom out at root scope
<headius> which gets a filename at construction
<headius> I need to remember how we dynamically provided that in jit code and use the same for populating static scopes
<enebo[m]> hmm. The wrinkles in this hurt
<headius> LDC "0;blah.rb;;-1;255;7;NONE"
<headius> ACONST_NULL
<headius> INVOKESTATIC org/jruby/runtime/Helpers.restoreScope (Ljava/lang/String;Lorg/jruby/parser/StaticScope;)Lorg/jruby/parser/StaticScope;
<headius> heh
<headius> oh
<headius> 9.2 did not serialize the scope with jitted code
<headius> so it always had to be given a live scope
<headius> now that it serializes the scope, the filename goes along with it and is impossible to update
<headius> but he said he tracked it to your commit, hmm
<enebo[m]> well that was likely the first commit to instantiate the staticscope
<headius> I think I can modify the "run" entry point in the jitted class to accept a filename
<headius> load/require/execute logic will just pass it through
<enebo[m]> yeah that sounds like a more explicit way to handle this
<enebo[m]> My mind is fuzzy. Original motivation was to not ask IRScope for runtime info. And staticscope had most of what IRScope had. I think the second level part of this was lifecycle of asking for info from IRScope before it had fully rebuilt itself from serialized/AOT state
<enebo[m]> This second level part though I think was more related to side-effect state which happened when instrs were un-persisted. That would not really include file.
<enebo[m]> This probably does not affect how you solve this but it is bugging me I don't remember this better. It was a lot of work
<headius> I think I see an easy path
<headius> when we load a compiled script we instantiate it as an AbstractScript < Script object that has a setFilename... I can add a String to the run entry point for the script and just pass that filename in
<headius> should be a minimal change and that run signature is only used here
<headius> this plumbing for launching a script has been on my to-do list for a rewrite... over-abstracted
<headius> ALOAD 2
<headius> INVOKEVIRTUAL org/jruby/parser/StaticScope.setFile (Ljava/lang/String;)V
<headius> that should be loading from the live filename now
<enebo[m]> cool
<headius> yeah messy all these entry points
<headius> and I had forgotten that AOT compiling just serializes the IR into a class file, so that path had to be tweaked to use the passed-in path as well
<enebo[m]> ship it
<headius> hmm this jar-dependencies thing has gotten sticky
<headius> so a user wanted to be able to update it independently of JRuby, but because we used it at boot it got activated right away and could not be upgraded
<headius> so he made a change to delay that activation, which lets us now use the newer version of jar-dependencies... but our build processes depend on released versions of JRuby which do not allow upgrading jar-dependencies and fail to run because it tries to activate the new one when the old one is already activated
<headius> I'm trying to figure out if there's a way to force it to only activate the old version for build-time purposes
<JasonLunn[m]> <headius> "In here works fine" <- There are headwinds to fully committing to an FFI-based solution for protobuf due to https://github.com/ffi/ffi/issues/753
<headius> Haha, I saw that come across my desk but did not know it was related to this effort
<JasonLunn[m]> So earlier this year it turns out that issue was referenced because the current protobuf CRuby code was doing something similar... malloc'ing memory without the Ruby GC knowing about it
<JasonLunn[m]> and so the CRuby GC wasn't running often enough.
<JasonLunn[m]> It wasn't a memory leak per se, but the user observable behavior was close enough that they decided to change our CRuby native implementation to better inform the GC.
<JasonLunn[m]> Right now, there is still appetite to switch over to my new FFI-based implementation for JRuby, which I think is still a win, but it isn't going to be the single implementation across interpreters that I was hoping for.
<headius> Does something similar exist for the Java implementation?
<headius> The memory information I mean
<JasonLunn[m]> As far as I can tell it doesn't, because you don't try to influence the Java GC
<headius> Any garbage collected run time is going to have the same issue
<headius> I'm not sure I understand why it's a problem for Ruby but not a problem for JVM
<JasonLunn[m]> I don't know how JNI/JFFI's allocation of memory via malloc impacts the JVM's decision to run GC or not
<JasonLunn[m]> I don't know if it is safe to say that the JVM doesn't care about those allocations in its GC pressure calculations
<JasonLunn[m]> I have to assume that since this issues has been open a couple of years without a lot of attention, other FFI-based gems are fine with the current behavior and that an FFI-based solution is still preferred for JRuby moving forward.
<JasonLunn[m]> Do you share that take?
<headius> This is not really a new issue. Both FFI and native extension solutions can allocate memory that the GC does not see and does not consider in its decisions. I will review that issue and try to add some comments but I would say that FFI is still preferred
<JasonLunn[m]> 👍️
<headius> There has been discussion about how to inform the JVM that objects have native memory attached but I'm not sure it's gone anywhere. The best recommendation is usually to manually terminate that memory when you know it's out of scope
<JasonLunn[m]> Fair enough
<JasonLunn[m]> https://github.com/protocolbuffers/protobuf/issues/9546 is the corresponding protobuf issue that's linked to the FFI issue.
<headius> Ok
<JasonLunn[m]> Even if the memory pressure issue were solved tomorrow, the other big source of resistence I'm getting to replacing the existing CRuby implementation is performance.
<headius> I would be interested to see if the JVM implementation has done anything in this area because if so, the same fix could apply to JRuby FFI
<headius> Oh yes I was wondering when that would come up
<JasonLunn[m]> The synthetic benchmarks we whipped up once the FFI branch was passing conformance tests were not kind
<headius> There's definitely some overhead in FFI but some of that is due to lack of optimization
<JasonLunn[m]> I was looking at some ways I might be able to move more of the implementation back into C to preserve more of the original performance characteristics
<headius> That sort of hybrid approach works pretty well too
<JasonLunn[m]> One of the original reasons I tried to rewrite as much of the FFI version rather than just reuse the code from the current CRuby implementation was exception raising
<JasonLunn[m]> I started down the path of the mental exercise of thinking about using FFI callbacks to let ruby do the exception `raise`'ing
<JasonLunn[m]> But I found some discussion somewhere that suggested that exceptions raised by FFI callbacks get swallowed. Does that sound accurate to you?
subbu has quit [Read error: Connection reset by peer]
subbu_ has joined #jruby
<JasonLunn[m]> I don't think https://github.com/ffi/ffi/issues/141 was the conversation I originally found, but I think it gets to the heart of the point
<JasonLunn[m]> There are a few places where I make multiple FFI calls rather than one, because I have to break up the logic around any point where I might need to do exception raising,
<headius> Hmm new to me but this may have been around a long time
subbu_ is now known as subbu
subbu has quit [Quit: Leaving]
Guest5161 has joined #jruby
Guest5161 has quit [Quit: Leaving]
subbu has joined #jruby
subbu has quit [Ping timeout: 268 seconds]