<enebo[m]>
headius: so getFrameName might now start returning the combined name?
<enebo[m]>
I largely just skimmed to the combining bit
<enebo[m]>
I half wonder if we should just put those two methods onto ThreadContext (getCalleeName/getSuperName) and then deprecate getFrameName
<headius>
enebo: I did that later
<headius>
all uses of getFrameName now use one or the other but I didn't deprecate it, I should do that
<enebo[m]>
oh I did not see that in the PR
<headius>
I force pushed a few times
<headius>
you might have seen an older version
<enebo[m]>
HAHA
<enebo[m]>
does github just update things on a force
<enebo[m]>
I swear I was just looking at a different diff like a minute ago
<enebo[m]>
I did not hit reload either
<headius>
when did you open it? Most of that was from last night I think
<headius>
ah yeah they do have some refresh pings
<enebo[m]>
lol. I have no idea but the stuff I said I thought was there and then when I went back it was all context
<enebo[m]>
funny
<headius>
interesting, I see a few places left that use getFrameName... I will fix them and deprecate I guess
<enebo[m]>
yeah otherwise we will get a very strange bug report one day
<headius>
some of the errors along the way were fun
<enebo[m]>
because I will definitely use it not remembering
<headius>
no such method "\0foo\0bar"
<enebo[m]>
yeah
<enebo[m]>
for me I may use it for debugging print statements
<headius>
I think I have everything green now though
<enebo[m]>
I think I am going to be working on ARJDBC until probably you are back just to try and get this done
<headius>
ok some of these are valid, just propagating whatever the frame name is
<enebo[m]>
I realize in the past for most updates it is just syncing a few files but postgresql is not as aligned as mysql/sqlite3 and I think the changes are larger for 7
<headius>
I could make a new name for the method and still deprecate, or just leave this
<headius>
we doing a 9.4.2 next week?
<enebo[m]>
you will be able to help or no?
<headius>
yes some of the days
<headius>
we are planning to spend most days at the AirBnB by the pool
<enebo[m]>
headius: yeah let's set a day that works for you and I can pre-test so we can shorten the time
<enebo[m]>
you can IM me or what not if you don't know yet
<headius>
I don't have anything else for 9.4.2 right now
<headius>
ok
<headius>
I should try reverting ripper/lexer.rb and see how it runs with this branch
<enebo[m]>
you can help untangle arjdbc with me then or hit some simple specs
<enebo[m]>
oh yeah that is a primary reason for the work to not be out of sync with the only real world use case
<enebo[m]>
Someone did specifically report the issue as well but I was unclear how real it really was since he went big with it being a primary tool
<headius>
yeah hard to believe this feature dates back to 201
<headius>
2012
<headius>
I pushed reverted lexer, we'll see how it goes
<headius>
you know I juggled all these GHA jobs around to put longer-running jobs up front, but it seems like they end up getting scheduled randomly anyway
<enebo[m]>
it is funny you cannot order them
<enebo[m]>
I have noticed the Mac queue must be a tiny number of available workers
<enebo[m]>
I almost wonder if we should move those to some nightly job with notification if they fail
<headius>
stdlib passed with reverted lexer
<headius>
the mac queue is one worker
<headius>
I'm not sure if it's simple to run more or not but this is the donated system
<headius>
there's no docker isolation or anything so the jobs might step on each other
<headius>
sockets etc
<enebo[m]>
We could potentially run it less since I see it mark jobs as unfinished unless we only make a commit once every hour sort of thing
<headius>
yeah it was stalling for some reason recently but seems ok today
<headius>
now I am wondering what else we can embed into the frame name 😀
<enebo[m]>
perhaps in the last week or two it has been stressed by other projects using same physical hardware or something
<headius>
I think it's a dedicated machine but I can't be certain
<headius>
sitting in some rack somewhere
<enebo[m]>
kwarg positional descriptor
<headius>
yeah that was my first thought
<enebo[m]>
but with that said anything which is not used very much would be fine since it would not require decoding
<enebo[m]>
or less decoding
<enebo[m]>
but it should be bidirectional in not needing to be encoded either
<headius>
yeah this is fun but should be a temporary fix
<enebo[m]>
Can I merge your callee PR?
<enebo[m]>
headius: I it is very mildly possible this is a failure in AR unit tests
<enebo[m]>
I did wonder if it is possible anyone else is using getFrameName in an extension
<enebo[m]>
If so then it may be worth having 3 new methods and have getFrameName still just return getSuper
<headius>
that might be better
<headius>
I can switch the valid uses of getFrameName to getCompositeName or something
<headius>
trivial refactory... not sure about that name
<enebo[m]>
I think the name is ok but it is much better than using framename for intent
<enebo[m]>
my only other thought is getEncodedNames but if you figure out another thing to mangle into that then perhaps even using name is problematic
<enebo[m]>
but I guess we shouldn't get too bogged down on this. This last thought was just to make sure any external consumers do not suddenly get weird names
<headius>
yeah hopefully we can just remove this once we don't need to do this encoding
<headius>
famous last words... I'll be asking why we never got rid of it in 2033
<headius>
I'll go with getCompositeName
<headius>
ok that should do it
<enebo[m]>
cool
<headius>
looks pretty good
<headius>
enebo: if you are comfortable throwing this into 9.4.2 I'm fine with it
<headius>
that last change was a good idea, lowers risk of any extension getting the weird name
<headius>
so it's down to internal uses, and hopefully I found all those
<enebo[m]>
I think I am ok with that but it really only would have fallout if we were accessing that value by another method we are not thinking about
<enebo[m]>
getFrame().getName
<headius>
I audited those early in the patch but I'll have another look
<enebo[m]>
before PR only 3 other uses
<headius>
yeah they are all now just setting up another frame copy or binding
<enebo[m]>
but I suppose if someone calls getFrame().getName() in an extension they may have the same issue of getting a weird value
<headius>
propagating the composite name
<enebo[m]>
with that said I am not sure this is very likely
<headius>
I could extend the getCompositeName to this level of course but I dunno
<headius>
and then to Binding too
<enebo[m]>
yeah it would completely remove the risk but it also is embedding this semantically into frame
<enebo[m]>
but with that said Frame does make sense for the two names right?
<enebo[m]>
It has a super name and a callee name on it
<enebo[m]>
how it is stored is a different issue
<headius>
yeah there's just no way to pass along two names so it has to be composite somewhere on stack
<headius>
so then there's three forms of it everywhere
<headius>
I didn't decode it in frame because it may not get used and that would add allocation
<headius>
I mean decode it eagerly
<enebo[m]>
yeah I just mean it may be stored on frame as composite and then stored/accessed with weird method on Frame but all uses would frame.getCallee
<enebo[m]>
moving your logic into frame I suppose
<enebo[m]>
I don't know
<headius>
yeah pretty much
<headius>
but util methods still on TC to avoid code accessing frames directly
<enebo[m]>
Is frame in same package as TC?
<headius>
FWIW there's six uses of Frame.getName... five are internal to TC and one is in IRRuntimeHelpers.newFrameScopeBinding
<headius>
yes
<enebo[m]>
We cannot do this either but we should consider removing accessors from frame and make it package protected
<headius>
yeah not a bad thought at some point
<enebo[m]>
9.5 or something...I don't even know how we can advertise visibility change
<headius>
I have wondered about hiding it more or exposing it more, because escape analysis might be able to get rid of heap frames and scopes when things inline
<headius>
but depending on that puts us in a precarious position
<headius>
going to TC for it means it can never be eliminated
<enebo[m]>
our pseudo bump pointer thing is another issue with that
<headius>
right
<enebo[m]>
I find it interesting to consider sphaghetti again for some fields like self
<headius>
just pass everything on stack
<enebo[m]>
Not because it will solve that problem per se but because we will basically use the same self for like 10 calls in a row and stuff that value ni 10 places
<headius>
30 base arguments to every method
<enebo[m]>
well that is the ultimate until it isn't
<enebo[m]>
Wasn't Scala complaining about some limit on param length
<enebo[m]>
a bunch of these fields are only current value and last value
<enebo[m]>
so 10 params for Frame
<enebo[m]>
:)
<enebo[m]>
but not all need previous value so less
<headius>
yeah no problem
<enebo[m]>
callInfo too
<enebo[m]>
kwargs indfo
<enebo[m]>
for positional knowledge
<enebo[m]>
a few other bits and bobs but it would all be on the stack at least
<headius>
yeah Truffle is able to do this for you by automatically eliding their frame objects when things inline, if they inline
<headius>
so you just allocate a frame
<enebo[m]>
that is nice but we don't get that so I think we have massive param list
<headius>
that's odd, my last push broke one test
<headius>
maybe I untagged something that wasn't quite ready
<headius>
enebo: jeremyevans made an interesting discovery
<headius>
the time to parse a method is quadratic on the number of parameters (or variables) in both CRuby and JRuby
<headius>
I just traced it to StaticScope.findVariableName, which is a linear search and done for each new variable encountered
<headius>
example script: jruby -e 's = 8000.times.map{|i| "c#{i}"}.join(","); loop { t = Time.now; eval "def foo(#{s}) end"; puts Time.now - t }'
<headius>
I have a simple patch that uses a Map to track existing variables, but I don't know if we want to keep this map around forever
<enebo[m]>
I realized two things on this 1) it needs to give out a slot value which is just an int count incrementing 2) it probably also needs to maintain order found
<enebo[m]>
for 2 I am not positive this matters but my nose is twitching
<headius>
it would be nice if this were a Map<String, int> too
<enebo[m]>
yeah that's true
<headius>
int nextIndex = definedVariables.size; definedVariables.put(name, nextIndex); return nextIndex;
<headius>
eliminates all dynamic variable table stuff from StaticScope
<enebo[m]>
that is not really a big problem since lvars are eagerly made
<headius>
needs to be a linked hash map so the order reflects the index when we pull it out
<enebo[m]>
we are eager on things which will exist before they exist but it simplifies a lot of things
<enebo[m]>
yeah it can be and that does solve 2 and not require an int since we can use length
<headius>
LinkedHashMap<String, int> but we might have to implement it
<enebo[m]>
All the machinery of addVariable will disappear as well and just leverage (probably) how we load static scopes from IR persistence
<headius>
or accept the Integer objects at parse time
<enebo[m]>
we are accepting Integer today but I guess we could save on those
<headius>
we are already paying integer cost today that's true
<headius>
so it's still a net reduction in alloc
<enebo[m]>
I think the main plus here is just a single alloc in staticscope
<headius>
right
<enebo[m]>
but linkedhashmap has to be cheaper than all that (although we still need a Map of some kind for other reasons)
<enebo[m]>
the lookup for nearly all methods is fine being linear
<headius>
LHM might cost more than what we're populating now in definedVariables, not sure how that cost will break down
<enebo[m]>
so I doubt that part of things is a visible cost unless you are jeremy :)
<headius>
I don't think LHM is much more memory than HM though
<headius>
just another field or walking nodes
<headius>
or=for
<enebo[m]>
unless what we choose creates more :)
<enebo[m]>
in any case I think removing some temp object churn is a good motivation
<enebo[m]>
at worst case I maintain a larger array and balance it :)
<headius>
yeah
<headius>
do it
<enebo[m]>
haha
<headius>
callee CI is back to green
<enebo[m]>
actually that would be very simple
<headius>
test_callee got untagged because it ran locally but --dev breaks in define_method
<headius>
so we are not 100% on this feature but probably 99% level
<headius>
broken = unoptimized (or unoptimizable) define_method plus __callee__
<headius>
that's all I know of
<enebo[m]>
one larger array which is unlikely to grow but would still require that which maintains order and has an int for size. The set still for the thing happening today. one array copy to mass assign to staticscope
<headius>
yeah that's another option
<headius>
since we need a linear array eventually anyway
<enebo[m]>
A single alloc of temp 50 String[] is not a big thing
<enebo[m]>
and who knows how small it could be but that would be something we would not want to ever resize normally
<enebo[m]>
anyways there are multiple ways
<enebo[m]>
The non-linear lookup I now think could be a bigger problem
<headius>
oh?
<enebo[m]>
I only considered parameters
<headius>
yeah it's all vars
<enebo[m]>
so I think we easily see methods with 50+ lvars
<enebo[m]>
most have very few or none but it is not that uncommon to see a method with 30 vars in it
<headius>
yeah but we don't constantly recompile them
<enebo[m]>
maybe a little uncommon
<headius>
so it's a one-time cost other than runtime generation of methods
<headius>
but it's a cost
<headius>
maybe significant across a lot of code in a codebase
<enebo[m]>
yeah maybe
<enebo[m]>
I think it could be more than I originally thought but I am unclear if you could measure it in the scheme of loading and running code
<headius>
well the total cost of this benchmark is the n log n linear searches for new vars and the n log n copying of variables as the array grows
<headius>
on top of the other map we already have
<enebo[m]>
yeah
<enebo[m]>
I just don't know how big of a deal that is in practice
<headius>
looks like only two new failures from removing --dev from CI jobs
<headius>
one is a source line off by one and the other is some frozen error
<enebo[m]>
Sorry I missed the details of that no --dev or just a bunch removed?
<headius>
we were passing --dev to a bunch of suites that wanted to jit
<enebo[m]>
oh hahah
<enebo[m]>
yeah I can see how that happened
<headius>
yeah I noticed it because this callee thing only failed with --dev locally, but it was failing in the jit jobs
<enebo[m]>
That's one way to make them pass
<headius>
it wasn't as an env var but clearly was propagating into MRI tests
<headius>
the string freezing one might be tricky since we don't guarantee the string will be the same if it runs in interpreter and then against in bytecode
<headius>
callee is super green, should we go for it?
<headius>
has not run with new --dev change thought 😀
<headius>
s/thought/though/
<enebo[m]>
merge callee
<enebo[m]>
I see a failed test in AR which is unlikely to be from that but it just happens to use callee
<headius>
it is done
<headius>
so I'm looking at these jit failures
<headius>
one is some enumerator arity thing that's due to how jit routes calls differently... low priority but should be fixed
<headius>
one is source location off by one
<headius>
one is frozen non-interned string is not same string the next time because of jit
<headius>
the only two that seem of possible concern are due to this:
<headius>
[] jruby $ jruby -e 'def foo(&b); lambda(&b); end; def bar(&b); foo(&b); end; p bar { }.lambda?'
<headius>
[] jruby $ jruby -X-C -e 'def foo(&b); lambda(&b); end; def bar(&b); foo(&b); end; p bar { }.lambda?'
<headius>
true
<headius>
false
<headius>
I mean they should all be fixed but for 9.4.2?
<enebo[m]>
they were already broken right?
<enebo[m]>
we just noticed them not working today
<headius>
ye
<headius>
kanye
<enebo[m]>
yezus
<headius>
yes, just exposed now in this one suite
<enebo[m]>
yeah I don't think it matters but it would be nice if they are not horribly hard
<enebo[m]>
What the hell is this test
<headius>
I'll see what I can do
<enebo[m]>
Is that lambda test meaning that the .lambda is on the result?
<enebo[m]>
if so shouldn't it print true?
<headius>
so near as I can tell if the block gets reified into a proc this way it should not become a lambda when passed to lambda
<headius>
simple and full do reify_closure
<headius>
JIT does not, I think because we run that &b passthrough optimization
<enebo[m]>
can't any proc become a lambda from being put into lambca?
<headius>
apparently not
<enebo[m]>
heh
<headius>
false
<headius>
$ jruby -e 'p lambda(&proc{}).lambda?'
<headius>
once a proc always a proc
<enebo[m]>
very weird
<headius>
this optimization seems to be runinng for full and JIT but full works
<headius>
so it's not actually optimizing delegation there
<headius>
and full still has reify_closure
<headius>
it's OptimizedDelegationPass
<headius>
-d
<headius>
ahh it doesn't work in full because the local variable escapes
<headius>
but why, hmm
<headius>
it should be able to be a temp
<headius>
oh hahah
<headius>
it runs out of order?
<headius>
these passes seem to be running in random order
<headius>
oh boy
<headius>
yeah
<headius>
full doesn't use any list of passes, it has a few hardcoded in a weird order
<headius>
two of the failures have patches; enumerator fix is simply removing some arities and autoload fix is to use real stack trace for line number (but that breaks some tests due to canonicalization)
<headius>
I've marked all the bugs and PRs for 9.4.3 but have a look at them