<enebo[m]>
puritylake: cool. Went dark all weekend.
<headius>
Good morning!
<headius>
enebo: been thinking through this callee thing a bit
<headius>
I'm wondering if we could narrow the frame name to the name of the method at the time we actually compile it
<headius>
I am trying to think of any cases where the name of the method to super changes and coming up with nothing... it seems to always be the original name of the method
<headius>
that would mean we don't use the frame name for super, and only put the called name there so we can just get it for callee
<headius>
`define_method(:bar, &method(:foo))` still supers up :foo too
<headius>
the same block used for multiple define_methods would have to be rewritten but we do some rewriting now anyway
<headius>
another possible way to store the alias callee without a stack would be to have the target method grab and clear it immediately
<headius>
enebo: I think this could work and possibly eliminate frame requirement for super in most cases
<headius>
at least for non-block cases
<headius>
so I see that we already do push the frame with the original method name when adding call protocol
<headius>
so there's no way for that frame name to be different than the original InterpreterContext name in full and JIT modes
<headius>
it will always super the original name
<headius>
unresolved super is the problematic one but when it occurs inside a module method I think it still should have a static super name
<headius>
module def foo; super; end; end will always super up :foo even though the super scoping will vary by hierarchy
<headius>
we should probably split unresolved super into two instructions, one for super within a block (which will need special attention or rewriting) and one for super in module method bodies (which can have a static name but needs to acquire the proper hierarchy for supering)
<enebo[m]>
ACP is only for full and JIT
<enebo[m]>
or is this that we always frame startup methods
<enebo[m]>
anyways yeah it seems like original name would be what we would call. Like if I alias foo bar and then super in foo it should execute bar and not some other method foo right?
<headius>
you mean if you super in bar?
<headius>
foo would be the alias in that
<enebo[m]>
I will make a snippet and see
<enebo[m]>
but I think it always just calls bar
<headius>
it seems like it always supers the original name of the method
<headius>
the only times we need to figure out name later are when it is a super in a block, either a normal block or a define_method block
<headius>
which could both be rewritten because it is still determinable at the time the method is actually constructed
<headius>
define_method(:foo) { super } will always super up :foo regardless of alias
<headius>
def foo; 1.times {super}; end will too
<headius>
modules need to query hierarchy to walk up
<headius>
the reason I bring up ACP is because it already statically sets the frame instr to use the IR method name at jit time
<enebo[m]>
I thought we did in cases already write original name
<headius>
that's what I'm saying
<headius>
instanceSuper has a static name
<headius>
but that only gets used for super in a method in a class
<headius>
it can also be static for super in a method in a module
<enebo[m]>
oh heh...we don't call the name because it has to still be known as super or it would call iself
<headius>
which is the only spec that fails if I make AliasMethod pass the alias name through
<enebo[m]>
ok yeah so instanceSuper is what I was thinking about
<enebo[m]>
In my brain I was thinking we just did a callinstr but that would not work
<headius>
yeah needs to call superclass not current class
<enebo[m]>
heh zsuper does not pass name
<headius>
yeah I am unsure about that one... zsuper oddity should only be args
<headius>
there's nothing else that is different (name, hierarchy target)
<enebo[m]>
yeah I would think it would mirror the other supers
<enebo[m]>
It is also odd to me it is special
<enebo[m]>
We also pass the args in explicitly
<headius>
so the idea here, in total, would be that the frame name is static at compile time (and potentially this means most supers are static and don't need a frame) and the name we pass on the call stack would become the callee name
<headius>
then callee just needs a special instruction that uses that or calls whatever goofy user-defined callee normally
<enebo[m]>
hahaha
<enebo[m]>
yeah that is an interesting idea until we forget and reframe it
<enebo[m]>
but I suppose that is what tests are for
<enebo[m]>
I see zsuper also has weird logic where it tries to grab block out of frame
<enebo[m]>
Seems like that should not be needed
<headius>
yeah I consider that part of args
<headius>
but name should still be static for non-block super
<headius>
and hierarchy search should be same as unresolved
<enebo[m]>
I think so too
<enebo[m]>
but now I am weirded out at us having ZSuperInstr
<headius>
aha
<enebo[m]>
It basically just does unresolvedsuper other than that block logic
<headius>
so if I add a ModuleSuper that has a static name but dynamic hierarchy this can work
<enebo[m]>
BlockSuperInstr
<headius>
halfway between instance super and unresolved super
<enebo[m]>
the Z is meaningless
<enebo[m]>
since it has an arg list passed into the isntr
<headius>
it doesn't
<headius>
need the Z
<headius>
`def foo; 1.times { super() } end` is just an UnresolvedSuper, not a ZSuper
<headius>
ZSuper seems to only be used for zsuper in a block, so BlockZSuperInstr
<enebo[m]>
yeah but super() is just a super with no args which is not a zsuper
<headius>
right
<enebo[m]>
but the instr gets an arg list when we make it
<headius>
I'm saying the only place we use ZSuperInstr now is for a zsuper in a block
<enebo[m]>
I am only just talking about the name having z does not feel relevant
<headius>
it could be BlockSuper but that unresolved super above is also a block super, just not a zsuper
<headius>
so BlockZSuper is the fully-weird case that has dynamic name, args, block, and hierarchy target
<headius>
WeirdSuper
<enebo[m]>
Actually I don't even think out block zsuper logic is valid Ruby
<enebo[m]>
It should be
<enebo[m]>
but we calculate proper scope for args and I think MRI says nope
<headius>
I think they kick it out for define_method but not for normal block in a method body
<headius>
I have a POC of ModuleSuperInstr almost done
<headius>
it's just UnresolvedSuper with a static name
<headius>
this could be a big deal
<headius>
if we can get module super to also use the passed in class rather than going to frame, super in method body will no longer need frame at all in any situation
<enebo[m]>
ok yeah we calculate zsuper for define_method and they will not. We should probably eliminate this since they seem unlikely to ever adopt it
<enebo[m]>
I pointed out this can be solved to ko1 and he seemed to not believe me
<enebo[m]>
yeah well if true that will be great
<enebo[m]>
HAHAHA
<enebo[m]>
I removed that special logic almost 1 year ago to the day (360 days ago)
<enebo[m]>
So next time I bring up special logic remind me it has been removed
<headius>
hah
<headius>
so the other half of this is that I think even unresolved super can eliminate dynamic super name
<headius>
for a block in a method, the method has a static super name so the block does too
<headius>
for a block that is define_method it should be rewritten to the right name at define time
<headius>
super in eval might be the only sort of case that requires something special but we can work around that
<headius>
this just adds ModuleSuperInstr with static name
<headius>
we might need to get dynamic name out of UnresolvedSuper to proceed to the next step, which is changing the frame name to be callee name
<headius>
then we can support callee in all contexts
<enebo[m]>
ok.
<headius>
should make it much easier to optimize super calls and they will inline now
<headius>
ok I have an additional change that almost works
<headius>
a super within a normal block within a normal method can use the method's name statically for super
<headius>
but I can't detect cases like def foo; Class.new { define_method { super } }; end
<headius>
that looks to this patch like a normal super in a normal method (foo)
<enebo[m]>
headius: should that work?
<headius>
is there a way I could detect this at IR build time?
<headius>
well let me clarify something... define_method bodies can do normal super?
<headius>
non-zsuper
<headius>
right now the main reason we always use a fully unresolved super for a super-in-a-block is because of cases like this, I think
<headius>
ok yeah so only zsuper-in-define_method is disallowed in CRuby
<enebo[m]>
yeah so ordinary super I guess works
<headius>
pretend that snippit about is ordinary super then
<enebo[m]>
and zsuper throws when you try and call it
<enebo[m]>
so that likely is just replaced by an explicit throw
<enebo[m]>
yeah it definitely is looking for the method name in define_method
<enebo[m]>
so yeah we could try but not neccesarily be able to detect it
<headius>
more specifically is there a way I can detect that it is a super in a define_method earlier
<enebo[m]>
define_method(some_method) {
<headius>
because most cases with super in a block will not be define_method
<headius>
and they can use a static name
<enebo[m]>
so we can look at the closure and likely figured out there is a wrappedirclosure to a call which has a name define_method
<enebo[m]>
but we may not be able to figure out the actual name
<enebo[m]>
in cases where it is a simple operand we could
<headius>
I think some rewriting will be necessary
<headius>
It needs to compile as a block super that is static but if the block is ever used in define_method It gets rewritten to a different name based on whatever method name was defined
<enebo[m]>
I think if we assume we can detect this at build we could probably then make something to save the name somewhere in this case right before the closure is invoked
<headius>
Do we rewrite define method bodies in all cases right now?
<headius>
Or only when they can be turned into a normal method body?
<enebo[m]>
only some times
<enebo[m]>
define_method impl has the restrictions and I don't recall
<headius>
If we rewrote them all the time we could make these super instructions use a static name in every case
<enebo[m]>
Look at IRClosure.convertToMethod
<headius>
Rewrote in this case just meaning look for nested super instructions and set them to the new name
<enebo[m]>
we will not do it for zsuper :)
<enebo[m]>
which is funny since we do not allow them
<headius>
If we can do this then there's no super anywhere in the system that will have a dynamic name
<enebo[m]>
I guess I did not rip out all special logic after all
<headius>
That's the only prerequisite for us to pass the callee name in stack
<enebo[m]>
yeah I think the only tricky aspect is just making sure we do not have a concurrency issue
<enebo[m]>
but I guess I would hope that is already the case
<headius>
We must be cloning the block, no?
<headius>
I mean cloning the actual scope for it
<headius>
IR scope
<enebo[m]>
yeah I don't mean the instrs or something like that just having two inflight redefiniitiosn
<enebo[m]>
if (block.getBody() instanceof IRBlockBody &&
<enebo[m]>
runtime.getInstanceConfig().getCompileMode().shouldJIT()) { // FIXME: Once Interp and Mixed Methods are one class we can fix this to work in interp mode too.
<enebo[m]>
So that is another restriction
<headius>
That would basically be two calls to define method and the last would win
<enebo[m]>
with a another random Tom FIXME
<enebo[m]>
I can only guess we have no provision for swapping out the new method with the old one on interp
<headius>
So I think what I should do is add another pass that clones the block IR scope and rewrite super instructions to the new name, and we do that for all blocks passed to define method
<headius>
Then if we can go further and make it a normal method we use your existing logic
<headius>
By then it will already have the correct name
<enebo[m]>
yeah I think ignoring the method part of this and just making the closure make a new closure would be the 100% solution
<headius>
And for blocks that are never used in defined method they will just grab the static name at compile time
<enebo[m]>
We definitely need to get a new scope like it does here where it gets a new method
<enebo[m]>
since any closure could be used for more than one thing
<enebo[m]>
as weird as that may be
<headius>
For sure
<headius>
That is the case that means we have to clone and swap super names
<headius>
define_method(:foo, &some_proc)
<headius>
If that proc ess has a super we just need to make it use foo
<enebo[m]>
usesSuper/usesZSuper are methods on these scopes so we do not even need to try to do this if they are not present
<enebo[m]>
(and only super in this case)
<headius>
s/ess//
<enebo[m]>
that flag essentially also tracks lexical children closures too
<headius>
Yeah I guess what I'm saying is we need to eliminate any super that gets the name from the frame
<enebo[m]>
yeah I am just looking at what is needed or what we have to help
<enebo[m]>
we only have to bother rewriting if hasSuper
<headius>
Ok
<enebo[m]>
we technically could just have a pass-like method for this since we just walk scope instrs after cloning and all children (which I guess would also get cloned)
<headius>
I think the eval case is fine as well since we would compile it as if it were a closure inside the current method, which has already a static super target
<enebo[m]>
We probably clone even children with no changes so that if we inline (which arguably is pretty broken atm) we can naturally use those closures however we want
<headius>
I don't think I need a new instruction for this
<headius>
Just scan the block body for any super instructions and set them to the new name
<enebo[m]>
yeah and any children closures
<headius>
After cloning of course
<headius>
Right
<enebo[m]>
and clone all of them
<enebo[m]>
yeah
<enebo[m]>
this is just a pass/method with an input of the name
<enebo[m]>
output will just be cloned with those supers converted
<headius>
RetargetSuperPass
<enebo[m]>
sure
<headius>
Well this is exciting
<headius>
Assuming this all comes together then callee can just look at the given name passed on the JVM stack
<enebo[m]>
This is about the time I would like to know how often we super
<headius>
Or in the frame if necessary
<headius>
There would be a separate callee instruction when it is used inside of a block or eval
<headius>
Hell I'm just excited that we might be able to get inline caching back in place for super calls
<headius>
callee is a bonus
<enebo[m]>
583 super in activerecord liib for Rails 6.1
<headius>
How many of those in module methods? 😀
<headius>
I think we might be able to make super completely frameless for normal methods
<headius>
The name will now be static and the target class hierarchy should be getting passed in already
<enebo[m]>
grep is not that powerful :)
<enebo[m]>
but I suspect most of them are in modules since Rails is pretty module heavy
<headius>
That's what I was thinking too
<headius>
the module super change is green at least, moving on to block supers
<headius>
hmm cloning and rewriting the closure is not straightforward