<kares[m]>
JavaClass is meant to go away eventually, some methods are still used (internally) but most are not or only from also deprecated callers
<kares[m]>
been trying to supress some of the useless warnings there - not sure what the current status is will need to poke around a bit to see
<kares[m]>
where's this coming from, trying to get to Java 17 and it gets a lot of warnings?
<headius>
kares: take a look at the PR I merged, I managed to move most of the utility functions out to non-deprecated classes and eliminated pretty much all of the legacy JI warnings
<headius>
And yeah there are way more deprecation warnings on 17 or 19 but I could really only fix a few of those since they depend on newer APIs then we can use
<kares[m]>
okay - yeah I've seen the PR and started post-reviewing the changes ... so far so good
<kares[m]>
well done handling the cleanup!
<headius>
kares: thanks! If we jump to 17 for 9.5 we can fix nearly all of the Java deprecations
<headius>
enebo: so I was reviewing some of my PRs
<headius>
what I have now is basically everything except line, coverage, and c_* events
<headius>
the c_* events could possibly be added but since they have to wrap around Java calls they require a good bit of indy logic and would make the method stub objects bigger
<headius>
in fact this all makes generated bytecode bigger and makes indy necessary to avoid overhead when trace is off
<headius>
I could commit this still guarded behind --debug and just call it a win on lower-cost tracing but I don't know if that has much value
<headius>
so I dunno
<enebo[m]>
yeah
<headius>
c_call and c_return can be done with indy too, always on in the DynamicMethod stubs and wrapped around the target in indy mode
<enebo[m]>
I guess landing it in that form is least that can be done
<headius>
maybe I should instead do another pass trying to make indy always-on
<headius>
yeah
<enebo[m]>
My main concern is warmup and eventual speed
<headius>
I don't want to lose it but the additional bytecode for every method and block body is irreducible
<enebo[m]>
without reasonable measurement of impact it is hard to think we just roll with it
<enebo[m]>
but putting it in behind --debug at least gets the impl into the code base
<headius>
the original goal was to get call and c_call working for the tracing enhanced test asserts so we don't have to pass --debug (or don't warn when it is omitted) and this does not get c_call
<headius>
that enhanced assert just uses tracing to produce a better error message
<headius>
so...
<enebo[m]>
any thought on longer term idea of just passing line into callMethod ish stuff and hiding this all not in bytecode or indy?
<headius>
that's essentially what the indy c_call does... wraps the target with a hook checking call site that does nothing if hooks are not on
<enebo[m]>
It still translates to bytecode over all so there is a check but the actual mechanism to just keep it on the stack seems like something we could do once we put other stuff in that param list
<headius>
the non-indy version requires backtrace which jitted code does not emit
<enebo[m]>
err not in the generated version but somewhere
<headius>
the indy version of c_call basically just embeds the file and line into the call site metadata so it can be used if hooks are ever turned on
<headius>
it adds an additional safepointish thingy to every bound indy call
<headius>
"should" boil away when hooks are off but it has to get optimized to do that
<headius>
I dunno, I really would love to do always on hooks but without some sort of deopt it adds at least size and time... larger bytecode, more method handles, longer to optimize away
<enebo[m]>
yeah
<enebo[m]>
My concern is what cost is it to eliminate needing the flag
<enebo[m]>
personally it is mildly annoying but things like coverage tend to be in testing where adding a flag is not a big deal
<headius>
it's basically ALOAD + INVOKEDYNAMIC for every entry and exit of Ruby methods or blocks right now
<enebo[m]>
sounds significant
<enebo[m]>
since nearly all of Ruby is a call
<headius>
I'm more concerned with the size increase for small methods
<headius>
I think I'll commit it as is but still guard call/return/b_call/b_return behind the flag
<enebo[m]>
so what new stuff is on?
<headius>
raise and thread events will always be one now because I figured they're high cost anyway
<headius>
(still guarded behind a boolean check for events being on, so it only adds an if to normal case)
<enebo[m]>
ok
<headius>
ok
<enebo[m]>
going for a walk
<headius>
oh one other small bonus: this moves all trace hook management out of Ruby.java into a new class TraceEventManager
<headius>
so that apurtell who has the CVE concerns can't upgrade to 9.4.x because of a readline/reline issue in the hbase shell they are supporting
<headius>
I am not sure what to do
<headius>
I asked him to report those issues, but this puts pressure on us to reconsider updating Psych and SnakeYAML in 9.3.x
<headius>
enebo: a question about setting call info when you get back... I see we are setting it to 0 now all the time for non-kwarg calls but I'm wondering if there's any way to eliminate that
<enebo[m]>
headius: back
<headius>
yo
<enebo[m]>
callInfo is complicated
<headius>
I was just noticing it is around all === calls for case/when too as I revisit my bytecode reductions there
<enebo[m]>
ah yeah
<enebo[m]>
so there are a few things to think about with callInfo atm
<enebo[m]>
1. any method can accept an empty **kw and not complain
<enebo[m]>
2. I actually went the route of if/elsing on this in IR code itself so Ruby will not send that but that needs to change
<enebo[m]>
3. I want to kill most of those keywords=true for calls which merely forward kwargs to the next method
<enebo[m]>
4. We have not really formalized how we want methods to accept kwargs
<enebo[m]>
that last one is not so important for today but it is something
<headius>
yeah so really the ultimate path forward is passing the arguments along but that's a long project
<enebo[m]>
yeah
<headius>
so in the shorter term there's reducing setCallInfo on one hand and rolling into indy call sites on the other hand
<headius>
given that it's a single thread-local bit of information reducing it more gets trickier
<headius>
we have to set it and clear it or state will leak
<enebo[m]>
My current experiment was to shift kwarg state to second part of callInfo so forwarding could be eliminated
<enebo[m]>
but that is independent of any check to make sure we do not arity error on an empty kwrest
<headius>
right hmm
<enebo[m]>
so one access and one shift + set
<enebo[m]>
but that shift + set will pay for itself in brittleness of what we have
<headius>
I ask because I was wondering if we are clearing callInfo reliably do we need to set it to 0
<enebo[m]>
the if/else in IR itself does solve the empty thing largely but it very expensive compared to a bit check
<headius>
but that is assuming we clear it for certain after everywhere it should be consumed
<enebo[m]>
so if you don't set it and it stays the same value any next method after a native method thinks it recieved kwargs whether it did or not
<enebo[m]>
If you marked a bit on it saying I entered native (an idea I had considered) it would be possible to know something but you are still messing with that int
<enebo[m]>
I think MRI should have errored on empty kwargs
<enebo[m]>
mri31 -e 'def foo; "HEH" end; h = {}; p foo(**h)'
<enebo[m]>
This is dumb
<headius>
yeah I agree with that in principal
<enebo[m]>
and for us it means bifurcating a call at some point to not pass it or have all methods need to check for it
<enebo[m]>
Current design in Ruby code is to bifurcate in IR itself but I think it should get put into call impl
<headius>
what would you think of putting call info into CallBase as call metadata right now, so I could stick it in indy logic
<enebo[m]>
The original if/else happened when I thought you could flag hashes as keyword hashes
<headius>
it really is call metadata after all
<enebo[m]>
now all calls know if they receive kws and at times whether they are already empty
<headius>
all call instr invokes will have to set CallInfo but then I can embed it in indy and eliminate the extra bytecodes at least
<enebo[m]>
yeah it just was a potential opt but as it stands it is probably just more bytecode in JIT
<enebo[m]>
interp is slower but only if you explicitly are using kwargs at a site
<enebo[m]>
all receivers in Ruby pay no cost
<enebo[m]>
if we were RiR this would be a bit simpler too
<headius>
the two things I'd like to do tonight/tomorrow is both involve making calls smarter:
<enebo[m]>
removing bifurcation altogether would mean checking everywhere but it would be extremely rare and a flag bitwise op which compared to all the other machinery may not show up for much more than math microbenches
<headius>
1. call info as call metadata so I can include it with the call itself
<enebo[m]>
and probably not then
<headius>
2. boolean return value from dynamic calls
<enebo[m]>
For 1 callInfo is metadata already in callinstr
<enebo[m]>
so you have it except for the liveness
<headius>
aha ok
<enebo[m]>
and having to know whether on the caller side if it has a splat or not
<enebo[m]>
err callee
<enebo[m]>
hahah you know what I mean
<headius>
ah yes I se it in JVMVisitor.CallInstr
<headius>
so I can do the rest of that myself
<enebo[m]>
So my if/else was before that happened
<enebo[m]>
you can rip all that out if you wantr to put the empty test into call impls
<headius>
the boolean thing is interesting because every when has an additional aload + isTrue call right now and those would disappear
<enebo[m]>
That is funny. I noticed that a few months back
<headius>
not to mention branches and loops
<enebo[m]>
It was one of those of crud
<enebo[m]>
yeah I think I noticed it in branches
<enebo[m]>
not neccesarily ===
<headius>
indy sites will just wrap the return path with !(ret == false || ret == nil)
<enebo[m]>
so how do you envision that...specialized call1ObInstr?
<enebo[m]>
where b is boolean
<headius>
yeah
<headius>
we do have some support for boolean retval but it's in the unboxing stuff
<enebo[m]>
yeah nice. then interp should not need an interpret method so long as it is just marked as calloperation
<headius>
for some limited number of call forms, though, I think we can cover a large number of common isTrue checks
<headius>
=== will always be an arity 1 call
<enebo[m]>
So you also need to add specialized Branch*
<enebo[m]>
I said above you don't need to add interpret but you can and it will still work (albeit with boxing) because temp is Object
<enebo[m]>
boxing probably is cheaper than isTrue
<enebo[m]>
Actually I take that back the interp will not figure that out
<headius>
short term it would just be a bytecode reduction
<headius>
for JIT
<headius>
it could be done as a JIT-only pass
<headius>
so we don't introduce boolean boxing into interp
<enebo[m]>
If you assume you are not always true or false it should get rid of bimorphism
<enebo[m]>
unless it gets both impls back and says of one true and one false
<enebo[m]>
which perhaps it does
<headius>
longer term it makes it possible for us to have core methods return boolean rather than RubyBoolean and many such cases will then never even touch a RubyBoolean or do the nil/false check
<enebo[m]>
less bytecode for sure
<enebo[m]>
simple stack check for impl
<headius>
so like str.match? or fixnum == fixnum2 can return boolean and start to fold away better
<headius>
so I will do the callInfo change to indy sites first and then see what it would take to do the boolean thing
<headius>
at least for case/when because that's a big offender
<enebo[m]>
unrelated but I added some int instrs to support pattern matching
<enebo[m]>
where ArgsNum is Operand argsNum = new Integer(fixedArgsLength);
<enebo[m]>
so primitive math is possible if you feel you need it internally
<headius>
yeah nice
<headius>
that will be useful later when we want to move more core to ruby
<enebo[m]>
other things maybe not noticed is named labels
<headius>
for e.g. loops we know we want to just be int loops
<enebo[m]>
yeah so long as we can know the result is going to be an integer (well long I am pretty sure) we can
<enebo[m]>
in fact BInt exists already too
<headius>
yeah from the unboxing stuff
<enebo[m]>
no from pattern matching I think
<headius>
oh right
<headius>
there's similar instrs for unboxing
<enebo[m]>
I wonder how that stuff has held up
<headius>
I do need to get more familiar with pattern matching and see what I can do in jIT
<enebo[m]>
It is epic
<enebo[m]>
but there are simple patterns
<enebo[m]>
tbh it makes defined? look simple
<enebo[m]>
but the basic patterns could be done much more simply
<headius>
yeah I am keen to optimize them
<enebo[m]>
I believe realizing something is homogeneous during parse could give some weight to doing more opted stuff
<enebo[m]>
yeah in theory if you know all arms are 3-length array arms you could even just getaway with a single array length check and compare nothing
<enebo[m]>
but there are lots of opts possible
<enebo[m]>
unfortunately I don't know how common that is in real code
<enebo[m]>
most uses I have seen mix lots of patterns together
<enebo[m]>
even if just different arities (still that is optable as well)
<headius>
two quick optimizations on a new branch: setCallInfo(0) now just does clearCallInfo
<headius>
and an indy site so all callInfo in JIT are just load context ; indy