<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
<enebo[m]> I just want that delta to be small
<headius> that doesn't seem right
<headius> join the server thread and then close it?
<headius> spec runs to completion (with errors) if I switch that
<enebo[m]> Can you close a server twice?
<headius> hmm
<headius> it seems like it is supposed to handle one client and then be done
<enebo[m]> I am just wondering if this should already be closed and that is just there to nuke it from orbit
<headius> so perhaps it is not being done right
<enebo[m]> Is that in the test code or in the lib itself?
<headius> aha
<headius> ok this makes sense
<headius> the client never connects because of the Hash error
<headius> so then it joins a server that should be shutting down but isn't
<headius> so I'm not sure how this was running to completion before but perhaps the shutdown expectations changed
<headius> it is a bit fragile this way though
<headius> I will file an issue
satyanash has quit [Quit: kthnxbai]
satyanash has joined #jruby
<rebelwarrior[m]> This is interesting new version installs using rbenv but throws errors in the process that don't seem to matter. Just an FYI. It's working fine so far.... (full message at https://libera.ems.host/_matrix/media/r0/download/libera.chat/9f4a0128172089ab0c77c43d85b821fd07d37084)
<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> zing
<headius> enebo: 95% of the work was processing the args and kwargs
<headius> after that plumbing the timeout through was easy
<headius> we really could use a scan_args... so maybe I will take a side trip to do that
<enebo[m]> we do have some kwargs processor which marks a predefined array
<enebo[m]> You wrote it I think. It is not as cool as scan_args but we don't really have pointers
<headius> yeah
<headius> Arity.scanArgs returns an array with the processed args but doesn't process much other than that
<headius> it just nils out anything optional that was not passed and returns a total args array
<headius> a first pass at a better version would return an object "AllArgs" that contains required, optional, rest, keywords, and kwrest
<enebo[m]> Annotation which generates a custom type with one field per named param
<enebo[m]> Although we will pay some price to box out parameter parsing
<headius> we need to expand the descriptor for methods to include specific keywords and kwrest
<headius> that will need to come along in order to generate optimized call paths anyway
<headius> boxing maybe... a simple box could be elided but that is not 100% reliable
<enebo[m]> value objects
<headius> yeah
<enebo[m]> Java 17+ but I suppose we could do something conditional too
<headius> value objects would allow passing multiple return values, which combined with destructuring would basically make out params
<enebo[m]> yeah
<headius> I started poking at a version that returns a bitmask and then you pass the bitmask in for each argument to get its real or default value
<enebo[m]> So I do think one blue sky here is to define the full signature in the annotation
<headius> int argDesc = scanArgs(args, min, max, keywords=true)
<enebo[m]> sure just document you can only have 32 parameters
<headius> name = extractArg(args, argDesc, index, default)
<headius> it's chatty though
<enebo[m]> It can be long too though
<headius> default probably wants to be a lambda too but avoiding allocation is hard then
<enebo[m]> I suppose it is debateable whether native java impls need to map to how JIT binds params
<enebo[m]> we have no native methods which accept more than I think 14 args
<headius> we could post-process code like scanArgs(args, "descriptor", var1, var2, var3) to expand to a line-by-line form 😀
<headius> marrying this with arity splitting is the holy grail so the plumbing would have to work within dispatch path too
<enebo[m]> yeah
<enebo[m]> ultimately this wound need to be aware of position passing of kwargs from dispatch path
<headius> scanArgs(args, desc, (var1, var2, var3) -> { rest of method })
<enebo[m]> heh
<headius> if we passed in self and context it could be a static lambda
<headius> effectively static anyway
<enebo[m]> I like it but this feels like @JRubyMethod encapsulating this could all be in the populator
<headius> a few overloaded versions with different arities would cover most cases
<headius> yeah but this is really just like having an intermediate arg handler that dispatches to arity-specific versions
<headius> JRubyMethod generation would use the same descriptor and logic to redispatch the right way
<headius> it's one possibility at least
<enebo[m]> yeah I guess I can see this from multiple angles but I really wish we had a language for this that was not Java :)
<enebo[m]> Even just multimethods to Have plus(IRubyObject arg) and plus(RubyFixnum arg)
<enebo[m]> but obviously it would be quite a bit more than just allowing narrow and wide types to go to the right place
<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> default boolean respondsTo(ThreadContext context, String name) { return respondsTo(name); }
<headius> `RubyString localHost = convertOrNull(context, localOrOpts, RubyTCPSocket::convertToString);`
<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> not sure of the use case here
<JasonLunn[m]> Ported back to the example in the issue; I'll try the getMetacClass variant
<headius> yeah sorry, I was doing this from memory a bit
<JasonLunn[m]> No problem. I'm getting the same results binding it this way -
<JasonLunn[m]> One argument nothing happens
<JasonLunn[m]> Two arguments, I get my custom message
<JasonLunn[m]> 0 or 3+ args, arg error
<JasonLunn[m]> I'll add the second example to the issue
<headius> you have something simple I can build and run to test this?
<JasonLunn[m]> Yeah
<JasonLunn[m]> Just updated https://github.com/protocolbuffers/protobuf/issues/9668 with the static binding version
<headius> ok
<headius> $ jruby -r ./respond_to_java.jar -e 'Foo::Bar.new.respond_to? :foo'
<headius> Custom repond_to? got called!
<headius> $ jruby -r ./respond_to_java.jar -e 'Foo::Bar.new.respond_to? :foo, false'
<headius> Custom repond_to? got called!
<headius> seems to work ok
<headius> that is using your second example as is
<headius> Jason Lunn: ^
<JasonLunn[m]> Call it with one argument
<headius> see above, I did that
<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> barCls.searchMethod("respond_to?").setBuiltin(false)
<headius> Or something
<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
<enebo[m]> 2009
<headius> These other definitions would be nice to have but perhaps putting the cart before the host
<headius> Or maybe we add another flag called isCore
<headius> Dunno
<enebo[m]> As I said if we change this signature in a way to determine that later we can add that later when we want to use that as a feature
<enebo[m]> but I guess that means we need to have an idea of how that check would work
<enebo[m]> HAHAH can this be true...it only has two consumers?
<enebo[m]> wait...I keep forgetting we have this on method and basicobject
<enebo[m]> basic object will just eventually meander into method
<headius> Yeah I think we should adjust this to mean it is a method bound during our core boot logic and consider other use cases as they come up