<i8her8oat[m]>
does anyone get this error? its for an FXCollections.osberbable_map
<i8her8oat[m]>
TypeError: illegal access on 'addListener': class org.jruby.javasupport.JavaMethod cannot access class com.sun.javafx.collections.ObservableMapWrapper (in module javafx.base) because module javafx.base does not export com.sun.javafx.collections to unnamed module @abc0923
<i8her8oat[m]>
add_change_listener at uri:classloader:/jrubyfx/core_ext/observable_value.rb:121
<i8her8oat[m]>
for <x = observable_array_list()> it work just fine
<i8her8oat[m]>
I forgot to say that the error is produced when calling add_listener
<i8her8oat[m]>
its from jrubyfx
<headius[m]>
Hmm well looks like we are trying to call a method using that non-public class as a target. Instead of maybe a superclass. So that call is within jrubyfx yeah?
<i8her8oat[m]>
not really, I think its within the jruby jar
<headius[m]>
Is there more to that error, like a stack trace?
<i8her8oat[m]>
its when I evaluate my ruby code, so I don't know if there is a method for ruby stacktrace or smt
ur5us has joined #jruby
subbu|bridge is now known as subbu
sagax has quit [Read error: Connection reset by peer]
sagax has joined #jruby
mistergibson has joined #jruby
<headius[m]>
i8her8oat: so that's all the output you get
<headius[m]>
if you are not on latest JRuby release you might try that... this appears to be related to Java modules
<headius[m]>
pass that to the JVM and the class will be visible, but we may not be handling this case right in JRuby
<headius[m]>
so file a bug about it
<headius[m]>
if you have a reproduction that would be a big help
<headius[m]>
basically, they are not exporting this class from the module, but the way we set up the methods from the interface means we're trying to bind directly to the class
<headius[m]>
even though the interface is public and exported, the class is public but NOT exported, and we shouldn't try to reference it direcly
<headius[m]>
modules modules modules, such fun
<headius[m]>
jmalves: I will have a PR for you to try momentarily
<i8her8oat[m]>
oh nice its working!
<headius[m]>
maaaagic
<i8her8oat[m]>
its like saying the word neural network ... it really is magic
<headius[m]>
are you on JRuby 9.2.11ish?
<i8her8oat[m]>
yes, latest version
<headius[m]>
ok then definitely open an issue... we're not handling this case correctly
<i8her8oat[m]>
jruby-complete-9.2.11.1
<headius[m]>
modules are so much "fun"
<headius[m]>
we are doing a .12 release so I may be able to patch this up for that
<i8her8oat[m]>
ok, i'll add an issue as reminder
<headius[m]>
if you can come up with a small example that would be great, but I have a general picture of the problem
_whitelogger has joined #jruby
ur5us has quit [Ping timeout: 260 seconds]
drbobbeaty has joined #jruby
<kares[m]>
pushed and updated issue + PR to have the 9.2.12 milestone
<kares[m]>
oh right, we might need the dynamic require (from RGs) to work
nirvdrum has joined #jruby
subbu is now known as subbu|lunch
<headius[m]>
enebo: I think you're right about returning ic from ensureInstrsReady
<headius[m]>
now that it has been modified to only ever update interpreterContext it will only ever return that one
<enebo[m]>
ok that was my only question too.
<headius[m]>
it doesn't matter really but no need to access it twice
<chrisseaton[m]1>
I struggle to understand those transitions as well
<headius[m]>
yeah it's grown to be overcomplicated
<chrisseaton[m]1>
I wish the IR container was immutable and transformations returned a new one
<headius[m]>
that's the plan for master, but it's too big for .12
<headius[m]>
so this change isolates full from non-full as far as the transitions go, and seems to avoid any issue
<enebo[m]>
chrisseaton: it was a premature optimization to change stuff in place in IR. It is slowly being undone
<enebo[m]>
but it also has caused issues over time especially for things like ir flags
<headius[m]>
dunno if you saw my message yesterday enebo but I realized why I did lazy instrs inside IC... we don't store instrs anywhere else
<headius[m]>
so there's a missing immutable instr + flags object here
<enebo[m]>
Yeah but the original design would just add them when constructing the object so I guess it makes sense to delay IC creation until we actually are going to use IC and the instructions contained within
<headius[m]>
right, which means we need to store them somewhere else, ideally somewhere immutable
<enebo[m]>
So that is a roundabout way of saying what you just said I guess
<headius[m]>
I started down that path a few times last night and it's doable but way too big for .12
<enebo[m]>
IC is literally just startup instrs as well so the notion that we wanted to be lazy on IR loading is an anomoly not thought of
<headius[m]>
thankfully the only problematic flag here is call protocol, which we can infer from the type of IC
<headius[m]>
so it's a dodge, but it's a trivial patch
<enebo[m]>
well just the fact that we pretend both can actualy return that is actually a little weird too
<enebo[m]>
only FIC can do passes
<enebo[m]>
the flag check part I am referring too mostly (from your PR)
<enebo[m]>
The original intent of IR serialize was to potentially also be able to straight decode to full but full decoding would be really expensive
<headius[m]>
there's other oddities, like FullInterpreterContext extending InterpreterContext even though they have completely different representations
<headius[m]>
so we'll fix up hierarchy, add immutable state for instrs + flags (+ cfg) and it will be fine
<enebo[m]>
well that is sort of the same issue
<enebo[m]>
They do have quite a bit of commonality but just the fact that instructions get nulled out is an indicator that they are not proper subtypes
<enebo[m]>
delivery is here...be back in a bit
<headius[m]>
yeah, I spiked an abstract superclass and it's easy and clean
<headius[m]>
pretty clearly shows how they would step on each other if they were overwriting the same flags or IC
<headius[m]>
enebo: I amended and forced that additional tweak
<headius[m]>
should be good to go now
<headius[m]>
jmalves: if you can confirm on your end that would be double good
<headius[m]>
so... kotlin support
<headius[m]>
🤔
<headius[m]>
I'm not sure the best approach for this... so far I can't find a way to distinguish a kotlin "companion object" from any old inner class + static field combo
<headius[m]>
basically the issue is that Kotlin defines companion objects using an inner class and a static field pointing at an instance of that class, and they have the same name
<headius[m]>
kalenp: are you around?
<kalenp[m]>
I am. Hello!
<headius[m]>
Can you explain what @JvmStatic changes?
<headius[m]>
I'm bringing up your example now
<kalenp[m]>
for methods which get annotated with it, it ensures that the method is defined as a static on the parent class in addition to a non-static on the inner class. presumably one points to the other, but I didn't get that deep
<headius[m]>
aha
<headius[m]>
so instead of it being MyClass.Companion.foo() you can also just do MyClass.foo()
<headius[m]>
but that doesn't work around this warning because the Companion name conflict is still there
<kalenp[m]>
correct. which is the java compatibility trick
<kalenp[m]>
yup, the interop is better (you don't need to include the companion when accessing it from java/jruby), but the warning is still there
<headius[m]>
so the problem is that I don't know that we can make a blanket prioritization of inner class over field or vice versa
<headius[m]>
hmmm
<headius[m]>
though we could just see if Java prioritizes one over the other
<kalenp[m]>
I saw in your original PR that you had some check for a kotlin annotation? That's beyond my knowledge of jvm internals, but is there something useful in that direction? Just tossing out ideas
<headius[m]>
you can't really do MyClass.Companion from Java and know which it will be either
<headius[m]>
yeah so we can definitely detect that this is kotlin, but I don't know if that's enough to know this is the companion pattern
<headius[m]>
there's no @KotlinCompanion on the companion class, basically
<headius[m]>
just a general annotation for all things kotlin
<kalenp[m]>
right. which would be the most explicit/useful thing for us here
<headius[m]>
I'll play with this from Java a bit and see what happens
<kalenp[m]>
I think the idea of "what does java do in this case?" is a good tie-breaker
<headius[m]>
Java may prefer the inner class or the field and if it does we can just use that as the rule
<headius[m]>
and it's the field, not the inner class
<headius[m]>
but I realize it's not ambiguous now
<headius[m]>
WeirdInners.Inner with nothing else like .class after it can only mean the field
<headius[m]>
new WeirdInners.Inner() is the class, as is WeirdInners::Inner.class
<headius[m]>
I think we do the field
<headius[m]>
for the extremely rare case where someone's doing this on purpose in Java code, you can still use java_import to rename the inner class
<headius[m]>
like `java_import("WeirdInners::Inner") { "WierdInnersInner" }`
<headius[m]>
I'd expect most such cases are intended to be some kind of weird singleton pattern anyway, so I doubt it will come up
<headius[m]>
enebo: I'm merging the OOB fix to 9.2, jmalves can test it from there
<enebo[m]>
headius: I see your change but I now question whether the ensureInstrReady need to be there either
<headius[m]>
that I can't answer
<enebo[m]>
Presumably promoteToFullBuild has happened before it could do this
<headius[m]>
I've never had a good handle on this workflow
<headius[m]>
that only fires if callCount >= 0, so not when lazy full is off
<headius[m]>
threshold=-1
<enebo[m]>
oh...it is in Mixed block body but interpreted does not call it
<headius[m]>
ah, yeah I do it before jit
<headius[m]>
I did try to reproduce this with methods and with regular jit and could not
<headius[m]>
only InterpretedIRBlockBody seems to be affected
<enebo[m]>
so mixed does not need it but interpreted might
<headius[m]>
they all do this transition slightly differently 🙄
<enebo[m]>
yeah a little bit
<enebo[m]>
completeBuild in interpreted it has to have happened
<enebo[m]>
well it could be called in both at completeBuild and we would know all direct paths in both do not need to call it
<enebo[m]>
headius: do you remember a long time ago I wanted to swap IC as being in front of IR Scope
<headius[m]>
enebo: yeah
<headius[m]>
it would make some sense
<kalenp[m]>
headius: Makes sense on the Inner class findings. We're going to try setting up an environment to test with the the nightly once all of this lands and make sure it's caught everything. Thanks again for putting this together!
<headius[m]>
I don't know if we need three objects or two for IR... IRScope (mutable but single atomic fields), ScopeData (immutable instrs + flags + cfg), and IC (immutable snapshot a single ScopeData with additional bits for interp)
<enebo[m]>
The work for Fosdem did mitigate it a little bit since IRScope is hardly used in the runteim at all now
<headius[m]>
yeah that is a big help
<headius[m]>
kalenp: excellent! Looks like the fix to prefer fields is a natural one-liner so I'll have a PR for you to try shortly
<headius[m]>
there will still be a warning but only in verbose mode
<enebo[m]>
Well I think IC is ScopeData but just needs to be immutable
<enebo[m]>
It was the original intention
<headius[m]>
enebo: it could be if we stop depending on it everywhere
<enebo[m]>
It contains instrs, flags which represent what was made
<enebo[m]>
but does not need to change it just needs to be made and done
<headius[m]>
you know what would really make sense.. a single "Executable" representationn that also does the jit transitions
<headius[m]>
and then we just reference that opaquely from IRMethod/Block/etc
<headius[m]>
keep all those changes under wraps and atomic
<headius[m]>
rather than reimplementing jit transitions all over the place
<headius[m]>
jit transition is just another instance of a pass
<enebo[m]>
startup, full are pure IR. but translation of a JIT from full or AOT from full could be just fine too
<enebo[m]>
in inliner there is optimized IC as well
<enebo[m]>
but IC was meant to keep each distinct transformation
<headius[m]>
I have already had to hack in a reference from scope to JIT bytecode
<headius[m]>
we might as well formalize that
<headius[m]>
so a given scope would have multiple states defined by passing instrs through passes or jit
<enebo[m]>
yeah the bytecode and handles going into scope may be a reasonable cause for having this sort of encapsulation
<headius[m]>
but you'd always get a snapshot when you request an IC from it, or whatever we call that immutable executable snapshot
<enebo[m]>
yeah I guess my only thought though is whether finished JIT thing really has much in common
<enebo[m]>
Like does a finished JIT form still have a CFG or most of the same flags?
<headius[m]>
well, it uses CFG and instrs to produce a bytecode representation
<headius[m]>
is that representation conceptually another IR state?
<headius[m]>
it's also possible to go backwards since it can serialize the IR into the bytecode
<enebo[m]>
I can see fullic or optic feed something which takes those as input and produces JIT/AOT thing
<headius[m]>
we have talked about deopt a lot and if we can deopt from bytecode back to IR I see bytecode as just another IR state
<headius[m]>
it just happens to use bytecode instead of linear instructions or CFG
<enebo[m]>
deopt as far as we discussed just falls back to full even if multiple opts have occured
<headius[m]>
right, but my point is that if they're all considered IR transitions then deopt fits more naturally
<headius[m]>
otherwise we'll end up having every place we use bytecode also have to think about deopt
<enebo[m]>
but I guess if we need to store state to dump temps or whatnot it could be that
<enebo[m]>
yeah I guess I don't know
<headius[m]>
just trying to centralize all transitions in one place
<enebo[m]>
I have also wondered about why IC is not directly a *Method at times
<enebo[m]>
It keeps some concepts separated but they largely just get stuffed into a method
<headius[m]>
yeah
<enebo[m]>
I guess though method table registration is one issue
<headius[m]>
this isn't a large change either in my mind
<headius[m]>
it just moves all this threshold checking and transition stuff into one location
<enebo[m]>
"foo" -> MixedMode which means not having to change method table
<headius[m]>
in truth every representation we have is mixed mode
<enebo[m]>
except AOT
<enebo[m]>
oh hehe I guess it is too
<enebo[m]>
It starts as startup
<enebo[m]>
yeah well so all of our *Methods can go from startup to full
<headius[m]>
even full bytecode AOT may eventually want to back off to IR interp if something goes awry
<headius[m]>
though my prototype is conservative and shouldn't have to
<enebo[m]>
Ultimately we should be able to make anything from full and it will be conservative
<enebo[m]>
but yeah it may go back to full interp
<enebo[m]>
but barring massive method or something like that AOT should always work
<headius[m]>
I was thinking along the lines of the optimistic call sites for bytecode AOT, using a profile feedback
<headius[m]>
my initial plan was just to include a direct call alongside the dynamic site, but that could be inlined, and then a back-off might be more important
<headius[m]>
but this is getting pretty far ahead
<enebo[m]>
yeah
<enebo[m]>
hey if we are going to make JIT/AOT use this we should also untangle JVMVisitor
<enebo[m]>
I would really love one instance per scope
<enebo[m]>
but perhaps that is just another topic altogether
<enebo[m]>
I added some simple peephole stuff and it ends up being some boolean set / check and hope nothing else is called between that set and check
<headius[m]>
yeah there's more abstraction than necessary there
<headius[m]>
there's like six different state objects during bytecode jit
<enebo[m]>
Well abstraction too but the notion that you may hit an IRWrapped Closure and then use the same visitor to process it
<enebo[m]>
which is different than methoddata and the other stuff
<enebo[m]>
I do have a patch somewhere where I tried
<enebo[m]>
...and I almost made it...
<enebo[m]>
I just remember the main problem was the highest scope was an issue
<headius[m]>
ah yeah
<headius[m]>
mostly because they are outputting to the same .class
<headius[m]>
but it makes state management ugly with stacks and such
<enebo[m]>
something like starting/finalizing the unit needs the same or different (cannot remember) info
<enebo[m]>
heh it was a while ago
<enebo[m]>
ok I am off on a tangent but I was thinking about the large method plan and this ended up making that idea more difficult
<headius[m]>
yeah chunking would be easier if the line between bytecode and IR were thinner
<headius[m]>
another reason I see it as just another representation
<headius[m]>
ok this is a bit more involved than I thought... just setting up the proxy class for the inner class tries to define the constant
<headius[m]>
like get proxy class for inner => add to package module => defines constant on outer class
<headius[m]>
so the constant gets set up as both a side effect of creating the proxy and as part of defining the pieces of the proxy
<headius[m]>
it may be appropriate to remove the side effect for inner classes, since they aren't going on a package
<headius[m]>
and the declaring class has logic to add the constant (or not) already
<headius[m]>
there's no risk of a class conflict within a package, but there's a risk of a constant conflict for inner class vs static field... so the declaring class should be responsible for that decision
<headius[m]>
I'lll qualify the constant name with declaring class
<headius[m]>
warning: inner class "WeirdInners::Inner" conflicts with static field of same name
<headius[m]>
I never know how much information to provide... it's never enough
nirvdrum has quit [Ping timeout: 265 seconds]
<headius[m]>
kalenp: if y'all can come up with a little maven or gradle project that builds and tests some scenarios, we can include that directly into our suite
<headius[m]>
test everything you expect to work 🙂
<kalenp[m]>
sweet. I'll see what I can do. could you point me at an example of something similar so I know what this should look like?
<headius[m]>
the existing tests are pretty specifically java code, but you can see the specs we have under spec/java_integration
<headius[m]>
the classes are in fixtures/
<headius[m]>
it's largely up to you how you would test this... we can adapt whatever you come up with to fit into our build
<headius[m]>
oh, there's also JUnit tests in core/src/test, and if you want to drive a JRuby instance from a JUnit test that's fine too
<headius[m]>
the only tricky bit will be mixing kotlin into test build
<kalenp[m]>
yeah, I think we may be at the forefront of JRuby + Kotlin development at Looker 😆
<headius[m]>
I will also make a trivial test based on the example above
<headius[m]>
but that's just a mechanical test for that one case
<headius[m]>
we definitely would like to see more JRuby + Kotlin integration and have even considered using Kotlin ourselves
<headius[m]>
it's funny... five years ago we got bugs about integrating better with Scala. Now we get the same bugs for Kotlin
<headius[m]>
(we never considered using Scala 😉)
<kalenp[m]>
the preference towards immutability and data objects are nice. I've been seeing the other thread here about refactoring some of the internals to be immutable and while I don't follow the details, the sentiment is familiar
<headius[m]>
yeah that's our general trend overall... but as a runtime we have to be mindful of excessive allocation too
<headius[m]>
so there's a balance
<headius[m]>
100% immutable all the time creates loads of transient garbage
<kalenp[m]>
makes sense.
<headius[m]>
kalenp: same username on GH?
<kalenp[m]>
that's me
<headius[m]>
oh I see it in issue
<headius[m]>
ok
<kalenp[m]>
not a whole lot of Kalen's out there, so I can usually get this alias
<headius[m]>
I don't want to say Scala's mangling is gross, but...
<kalenp[m]>
Do you think it would be better to follow that example and create a java class with the expected pattern, or to actually bring in kotlin to test against the real deal?
<headius[m]>
Kotlin may change this structure, so I'd say the latter is best... but if we can represent these patterns in valid Java then it's a valid Java case too
<headius[m]>
in the Scala case it was simple enough to do the same structure in Java, but we're making an assumption that Scala won't change how it does this
alexej[m] has joined #jruby
<headius[m]>
I pushed the simple test case
nirvdrum has joined #jruby
<headius[m]>
sad truth is both the Scala and Kotlin patterns are working around the fact that Java doesn't have class objects like Ruby does
<headius[m]>
and with that we have no other .12 issues except for some deferrable fixes to the build 👍