<byteit101[m]>
enebo: I've updated jrubyfx master to use the new ivar names. Give it a whirl, and I'll likely snap the version and push 2.0 to rubygems Friday after work unless you encounter issues
<headius>
Good morning!
<enebo[m]>
byteit101: ok I will give it a shot
<headius>
before I shut down last night I tried running part of that TCPSocket#initialize spec and it errored as before, so I think I've figured out that it is timing out setting up the server
<headius>
that may be why the spec dies, because it hangs in a `before`
<headius>
that could be a regression so I'll try to look into getting it working today
<enebo[m]>
cool
<headius>
once we have wip running to completion I'll see what I can knock down
<enebo[m]>
I wasted hours yesterday changing the design of the DSL rewriter for generated ripper code
<headius>
hah refactoring black hole
<enebo[m]>
I will save what I have and just go back to what I had
<enebo[m]>
well I ran into this case where one method was doing all this subtitution then realized it sometimes happens in a second method but that second method uses eval and method_missing
<enebo[m]>
That had a big appeal because it basically breaks up the arguments into an array vs me trying to use regexps to get them
<enebo[m]>
but eval'ing evals with interleaved m_m is a total fucking nightmare
<headius>
yeah that doesn't sound fun
<enebo[m]>
So I will go back to what I was doing since all this script really has to do is just process what is there now
<enebo[m]>
Next time that code changes I might have to add another regexp maybe but the universe of things in that code is somewhat limited (and in 3 places I flat out just replaced it with my own code)
<enebo[m]>
which means I can just replace places the dsl cannot parse easily with something it can
<headius>
I really wish we had a good way to implement CRuby's scan_args
<headius>
having to do this argument counting every place we try to pick apart optional args and kwargs is messy and error-prone
<headius>
rebelwarrior: oh that is weird
<headius>
maybe you can dig into that and try to figure out where that `tr` is coming from
<headius>
probably worth a bug report to ruby-build but not sure if that's the problem or if it is rbenv processing output from it
* headius
ponders scan_args in Java while he gets another cup of coffee
<JasonLunn[m]>
Was #9668 reproducible for you, headius ?
<headius>
aha
<headius>
Jason Lunn: If you just want to define loose methods to add to a Ruby class that does not correspond to a specific Java class (e.g. RubyString is String), you should make them static and pass in receiver
<headius>
if you had a Bar class in Java with this respond_to? on it it would work fine, but as it is defined on a different object I'm guessing it is just calling the base respondsTo
<headius>
what you did is basically defining a new respond_to? on the Bar Ruby class by using the annotations from an unrelated class
<headius>
so it created the binding but doesn't call the one you expect it to
<headius>
could be an error in the binder for this 🤔
<headius>
hmm we could make a scan_args that uses context as temp storage for "out" vars but they would escape then
<headius>
not that we are very good about preventing that in the first place
<headius>
and they would have to be cleared after retrieval
<headius>
how likely is a little temp array to be escape-analyzed away I wonder? It would be created on return and immediately disassembled and abandoned
<JasonLunn[m]>
headius: I'll give that a try;
<JasonLunn[m]>
It's weird that it is such different behavior for the one vs two argument form
<headius>
hmm yeah I would be interested in seeing a stack trace from within respondTo for both cases
<headius>
I would not expect it to get called at all
<headius>
it may be a quirk in how Java dispatches to this method given the options
<headius>
it should try to cast the Bar to the Java type RespondTo, and fail at that point
<headius>
we really are defining our own sub-language with arity splitting as it is now
<headius>
keywords will take that much farther
<headius>
perhaps @JRubyMethod attached to a static class with a bunch of static method of different shapes... we bind them all as one method with several escape paths
<headius>
JRuby X bruh
<enebo[m]>
Jitanius
<headius>
hah
<headius>
usage of that method above is obviously trivialized for one arg but it would be like return withArgs(context, self, args, 0, 1, (name) -> puts(name));
<headius>
longer body would need {} but not look much different from the current logic minus all the arg wrangling at the beginning
<headius>
can't throw exceptions from lambda though so that would have to be tweaked
<headius>
(terrible side effect of checked exceptions)
<enebo[m]>
This function can also potentially be n functions too if it wants to be hooked up to specific arity call paths
<enebo[m]>
which if you remove min and max as parameters could then be a plethora of functions which do not have most switch paths
<enebo[m]>
this can be a refinement later obviously
<JasonLunn[m]>
headius: in the last line of your gist, did you mean to call `respondTo` on `self`? IRubyObject has some methods that called `respondsTo` instead, but none of them have a type signature that takes a context...
<headius>
ah yeah I meant one of those
<headius>
or explicitly cast to RubyBasicObject and use the context ones
<headius>
we have not wanted to add to the interface, which isn't actually useful to us now
<headius>
enebo: we could use default methods to add more utilities to IRubyObject perhaps
<headius>
we could be using lambdas more to eliminate a lot of boilerplate in any case
<headius>
(at risk of introducing code paths that HotSpot can't inline)
<JasonLunn[m]>
hmm. `return self.respondsTo(methodName) ? context.runtime.getTrue() : context.runtime.getFalse();` results in an infinite loop that eventually throws a stackoverflow
<headius>
show me the stack
<headius>
that should not be the case since the static method should call into an instance method
<headius>
it should not end up back in the static method
<headius>
not sure I can see enough of it but I don't see any RespondTo class in that chain
<headius>
this is not the example you posted though
<JasonLunn[m]>
Yeah I was trying to skip a step. Standby
<headius>
in the stack you showed it is calling back to respondTo in the RubyMessage object which tells me you are binding it correctly as an instance method in that case
<headius>
ah wait I see
<headius>
the base respondTo method redispatches to `respond_to?` again
<headius>
so that is not quite right
<headius>
ok a different way to do that without the dispatch: self.getMetaClass().respondsToMethod(name, true)
<headius>
true = only public
<headius>
sorry this is kinda back and forth... if you can push a small repo with an example, or point me to the code in grpc, I might be able to help more directly
<headius>
if you know the superclass will not have the method you don't really have to redispatch either
<headius>
are you sure you're running the second form?
<headius>
enebo: after fixing TCPSocket specs I found the same pattern in UDPSocket, where it expects the server to receive something and blindly joins it
<enebo[m]>
yay
<headius>
eregon seems to agree that explicitly closing it in shutdown makes sense so I will make that change locally and PR something
<JasonLunn[m]>
Yeah... I just changed the error message to double check
<headius>
I also had wip CI set up to run capi specs so I am fixing that, but I have a completing job now
<headius>
Jason Lunn: not sure what to tell ya... I copied the second example into the right file, built jar, and it works
<JasonLunn[m]>
Try it with `jruby -X-C -r ./respond_to_java.jar -w -e 'Foo::Bar.new.respond_to? :foo'`
<headius>
aha something with -X-C is messing it up here
<JasonLunn[m]>
Yeah I blindly copied the command line invocation from https://github.com/jruby/jruby/issues/3252 as I was grasping at straws yesterday trying to adapt a working basiclibraryservice
<JasonLunn[m]>
I don't actually remember what -X-C does
<headius>
this is weird
<headius>
it doesn't seem to call the base class respond_to either
<headius>
oh lordy
<headius>
enebo: ok this is sorta funny
<JasonLunn[m]>
Thanks for confirming I'm not crazy
<headius>
we have RespondToCallSite that tries to cache previous results
<JasonLunn[m]>
I've updated 9668 with a note about the variable behavior when using `-X-C` for the second example
<headius>
if it sees that the respond_to? is a built-in one then it will skip the dynamic lookup and go straight to the method table
<headius>
unfortunately isBuiltin just checks if the method is implemented in Java
<headius>
it should really be checking if it is the default impl using some core logic
<enebo[m]>
yeah
<enebo[m]>
It seems we just never considered this use case
<headius>
this is probably the first time someone has implemented respond_to? in Java without overriding the existing instance methods at the same time
<enebo[m]>
What is the cheapest mechanism to know this is a core native method?
<enebo[m]>
Knowing it is native is cheap which is likely why we did it
<headius>
currently, we would have to cache it somewhere at boot and compare
<enebo[m]>
We could make all native core methods an extended type
<enebo[m]>
NativeCoreMethod
<headius>
that's a possibility yes
<enebo[m]>
of whatever
<headius>
or make the annotation know about builtin and set it at boot
<enebo[m]>
but I guess part of the issue with that is yeah
<headius>
builtin = true all over seems gross, but @JRubyCoreMethod is maybe less gross
<enebo[m]>
the actual construction of the method needs to know it is really a code method
<enebo[m]>
err core
<headius>
I'm not sure what is the best workaround here 🤔
<enebo[m]>
Another way might be looking at type and making a class anno "type = "core""
<enebo[m]>
You can't reopen Java
<headius>
true
<JasonLunn[m]>
I've got to step away from keyboard for a couple of hours
<JasonLunn[m]>
I think I've captured both issues in the ticket -
<headius>
Jason Lunn: I will come up with a workaround for this
<JasonLunn[m]>
the original issue with the non-static method - that one is consistent regardless of the flags,
<headius>
I have to ask... does this need to be in Java or is it an optimization thing only?
<JasonLunn[m]>
and the second issue where the static method only works if -X-C isn't a flag
<JasonLunn[m]>
I'd be happy to talk through how we might do this outside of Java -
<headius>
-X-C uses the interpreter only, which is using this flawed call site logic
<JasonLunn[m]>
the method_missing is implemented in Java,
<JasonLunn[m]>
so it would be nice to have them adjacent
<headius>
the jit mode code must not be using the same path
<headius>
Jason Lunn: ok makes sense
<JasonLunn[m]>
This is all in the context of wrapping the protobuffer Java lib,
<headius>
but we need a general solution for what builtin means
<JasonLunn[m]>
so there are a number of things that we think we have to do in Java, but we could be wrong
<headius>
we can fix this in JRuby simply enough by not using this naive isBuiltin check for now
<headius>
brb lunch and thinking about this problem
<headius>
enebo: I will push updated wip in a second that should run to completion
<headius>
includes the socket shutdown change plus TCPSocket connect_timeout
<JasonLunn[m]>
ttfn
<headius>
One work around would be to retrieve the method and set it as not built in
<headius>
That's not pretty but it might be the fastest path forward to work around this bug in our respond to logic
<headius>
enebo: if we passed a core class into isBuiltin It should be able to check whether that is where the implementing method comes from
<headius>
It would be a more generic way of asking, does this method come from the base implementation in this class
<enebo[m]>
we have implementation class so that is not so bad
<enebo[m]>
on method that is
<headius>
That is the ruby class, which would work, but I was thinking of passing in a Java class
<headius>
But that brings up another use case, being able to tell when a method is from a base class and implemented in Ruby
<headius>
Oh, one problem with that is that if you overwrite it the implementation class will still point at the same thing
<enebo[m]>
So replace one native method with another into the Ruby class
<headius>
I think we're starting to dance around a couple different definitions of built-in
<headius>
There's built-in as in implemented in Java, which is what we have now and that breaks for user extensions
<enebo[m]>
builtin was originally meant to signify the original native definition so we could java dispatch to it if it wasn't redefined
<headius>
There's built-in as in shipped with JRuby but that doesn't cover cases where we override the same method in a few classes and the overrides should not be considered the base implementation
<enebo[m]>
Whether we use it for more I guess maybe means we overloaded it
<headius>
I think the most useful definition we're looking for is whether a given method corresponds to one specific piece of code in JRuby
<headius>
Since we're almost always using this to skip the Ruby logic
<enebo[m]>
piece of Java code or Java & Ruby code?
<enebo[m]>
I don'
<enebo[m]>
t think I care that much about Ruby in this case
<headius>
Could be either really as we move more stuff to Ruby
<enebo[m]>
Ultimately I guess it depends on how we use that knowledge
<headius>
The base method missing for example could probably be implemented in Ruby if we could tell it was a core version
<headius>
Mostly it just has to raise a specific error
<enebo[m]>
Hmm
<headius>
Jason Lunn: I am still pondering this but I think if you ask the bar class for the respond_to method object and setBuiltin(false) it will work
<headius>
Pulling us from memory so don't quote me on the exact names
<headius>
s/us/this/
<enebo[m]>
It is worth pointing out the caller of isBuiltin can do what it wants with that knowledge. I guess a broader definition is reasonable so long as this does not make it all more complicated
<headius>
Yeah in this case it's a bug in our respond to call site because it is assuming built-in means core
<headius>
And really it is assuming built-in means specifically the one we've defined on RubyKernel
<headius>
It just doesn't mean that unfortunately and we didn't realize
<enebo[m]>
we didn't need to
<enebo[m]>
I can see it is a problem now but it hasn't been for like 9 years
<enebo[m]>
maybe more
<headius>
Yeah
<enebo[m]>
so isBuiltin() -> true then we need to instanceof on method type to know Java or Ruby
<enebo[m]>
but today if we only fix for Java then it will only return true for Java
<enebo[m]>
One is a subset of the other and I think we can expand the definition so long as parameters to it can determine both
<enebo[m]>
but I don't know if we need to add both now
<headius>
I have logic that checks if method missing is literally the one defined on Kernel which has likely prevented this from breaking when people implement method missing in Java
<enebo[m]>
so this can actually help remove some special code
<headius>
I actually cache the method objects and do an identity comparison in the call site
<enebo[m]>
fn(JavaClass, Method) or fn(RubyMethodClass, Method)
<enebo[m]>
where RubyMethodClass I guess is actuall RubyModule
<enebo[m]>
if we pass in RubyModule then we only know it has a method defined there but we don't want to searchMethod to see if it changed
<headius>
The more I think about it, the more I realize what we want is to know if the method is specifically pointing at this one body of code, whether it's in Java or Ruby
<headius>
isBuiltin(RubyKernel.respond_to_p) sort of
<enebo[m]>
I think it adds more possibilities but for the sake of existing code we really only use it to bypass Ruby dispatch
<headius>
There are not too many of these cases, we could potentially have a stack of enums that represent all the things we want to check for built in status
<enebo[m]>
One ENUM
<headius>
IsBuiltin(Builtins.RESPOND_TO)
<headius>
@JRubyMethod(built-in = RESPOND_TO)
<enebo[m]>
yeah I guess so
<headius>
Another case to consider are built-in numeric operators
<headius>
The ones on float and the ones on integer are both built in so you would need an additional type check to short circuited
<enebo[m]>
My only reservation is people may think they can just use isBuiltin() and then forget it also needs an annotation to work
<headius>
Which we have right now, but the built-in check is a bit flawed
<enebo[m]>
Having all builtins being detectable reduces a potential coding mistake
<headius>
What is the definition of a built-in though?
<headius>
Something we ship in the box?
<enebo[m]>
It originally just meant is was the original native code method
<headius>
Yes
<headius>
We can fix that definition by only setting this flag during core boot
<headius>
Rather than for all Java bound methods
<headius>
That is what we originally intended for it to mean