<byteit101[m]>
Can I add a project/label for this work?
<headius>
yeah like what?
<headius>
we need to thin ouy the labels we have but mostly just removing some that are not useful
<headius>
thin out
joast has joined #jruby
<headius>
enebo: here's a good optimization we don't seem to be doing: multiple assignment doesn't need to create an array when it is not used as an expression
<enebo[m]>
ah that's true
<enebo[m]>
I plan on releasing new joni and I have to use java 9+ to do it right?
<enebo[m]>
because of module additions
<headius>
oh wait I think we do it right
<headius>
I was doing a, b, c = nil and for cases like that we do create the array
<headius>
maybe that should be reexamined
<enebo[m]>
I didn't look at how we do it but I am not surprised if we do not do all we can there
<headius>
when RHS is a simple expression there are certain easy things we could do
<headius>
yeah
<enebo[m]>
a, b, c = 1, 2, 3 should definitely just assign
<headius>
it does
<enebo[m]>
mismatched no doubt is where it is more complciated
<enebo[m]>
I don't recall does a, b = foo does splat right?
<enebo[m]>
so this should look like the copy version and not masgn since it assumes any mismatch must be doing some array accessing
<headius>
in the case where the values is not an array, but is a simple value, we can treat it as an array of [val, nil, ...]
<headius>
if the [val] array is needed it will remain but we know the other assignments will all get nil
<enebo[m]>
I don't think so
<enebo[m]>
I think IRBuilder is just building this to use masgn_reqd to begin with where the matched case builds the array but uses copy for the three assignments
<enebo[m]>
because it uses masgn_reqd we never figure out the array is not needed because that is how it grabs the value
<enebo[m]>
heh I need to look at passes now I do not see how this is converted to a copy in builder
<headius>
because it translates itself into a simple operand
<enebo[m]>
local optimizations does it
<enebo[m]>
It looks really wonky because literals can propagate
<enebo[m]>
it would look a lot clearer if they stuck with the temps they were using
<headius>
I'm not sure if canCopyPropagate is equivalent to SideEffectFree but that's what I went with
<headius>
I think I might have broken the case where you do want the array in the end though
<headius>
so this to_ary logic is possibly why we never made this optimization
<headius>
but we can work around that
<enebo[m]>
if buildMultipleAsgn19 is where this toAry comes into being I think all you need to do is put an if there and else with making an Array literal containing the single value
<enebo[m]>
It makes the array so that is perhaps a waste but it is dead so it goes away
<headius>
aha
<enebo[m]>
canCopyPropagate may be equivalent but I don't know if that will trigger other stuff to prop
<headius>
it's ok
<enebo[m]>
you saw this will break some bizarre ruby behavior?
<headius>
I mean this might need to check to_ary on immediates but it doesn't have to create the array
<headius>
x = (a, b = 1)
<headius>
x gets 1
<headius>
not [1]
<headius>
so my change is still ok
<enebo[m]>
yum
<enebo[m]>
I hate that stuff
<enebo[m]>
but yeah last expr value
<headius>
looks much better in bytecode now
<enebo[m]>
you are effectively replicated LO logic there which perhaps is better? not sure
<enebo[m]>
I do wonder if this can be done in builder directly too
<headius>
yeah that's why I'm not sure if it could be simpler
<enebo[m]>
but perhaps that is stuffing too much into builder
<headius>
I don't think IR is seeing through the array contents
<enebo[m]>
It can be simpler if you make an Array(nil) at original masgn point but it then requires LO and DCE
<headius>
it is the builder making the assigns 1:1
<headius>
so it creates the array, puts it aside, and then assigns the values directly
<enebo[m]>
yeah
<headius>
rather than via an array
<headius>
before any passes
<headius>
this basically simulates that for single literal values
<enebo[m]>
This really makes me wonder how common masgn is and whether it would matter...we still have to determine whether something uses the Array in cmomon case
<enebo[m]>
So it can just be copy in interp but it also will still make the array unless we can do some low-grade analysis of whether it is used in IRBuilder
<enebo[m]>
so both LO and DCE for masgn could happen in builder but would it be worth it
<enebo[m]>
If you want to roll with this particular version you should add comment to both conditionals you added. In one year I may not understand why those are there
<enebo[m]>
the canPropagate one in particular
<enebo[m]>
I still yearn for some good analytics on what is called the most
<headius>
I think your original idea for this optimization is right now
<enebo[m]>
It is a little simpler and it could be in the AST generation except for the problem that this is all stuffed into bodies in the actual parser (so not as simple)
<headius>
I let the return value be whatever it is but I compile the assigns using a single-element Array operand
<headius>
that lets LO do the rest
<enebo[m]>
yeah
<enebo[m]>
I am not wondering though if we can do a poor man's DCE in IRBuilder
<enebo[m]>
normally we detect usage using MOP/SEEK pass to build up live variable info but I half wonder if we just bit fielded whether ANYTHING accesses a temp could immediately mark it as dead
<enebo[m]>
I guess variable is more than that since the rhs of what it is assigned to may have side-effects
<enebo[m]>
yeah that looks right to me
<headius>
my original version broke on a, b, c, *d because the rest of that logic expected an Array operand
<headius>
so this is simpler and works like a, b, c = 1, 2
<headius>
without creating the array at all
<headius>
I'm going to push this to my bytecode optz branch
<enebo[m]>
I suppose we could mark any variable accesses for temps in IRBuilder and at end of pass run we could see if the rhs has side-effects
<enebo[m]>
that would kill that array if it is not the return or some other variable puts it into parens
<enebo[m]>
foo((a, b, c = 1))
<headius>
this does give me another optimization idea
<headius>
which could possibly dovetail here to keep the to_ary call for literals
<headius>
ToAryInstr should include a builtin check and cached call to to_ary only if needed
<headius>
for any builtin to_ary it can just immediately create the array
<headius>
for some subset of builtin to_ary
<headius>
we might want to introduce an IR instruction for a call that has a separate branch if the method is built-in, like a primitive in a way
<headius>
oops
<headius>
SideEffectFree is too broad
<headius>
that pulls in things like local variables that may be pointing at something that must be to_ary
<headius>
I narrowed it to ILiteralNode
<enebo[m]>
oops
<enebo[m]>
ZArray may mess up
<headius>
aha
<enebo[m]>
That can extend ArrayNode and not lower ListNode
<headius>
yeah then it would be caught by the earlier type check
<enebo[m]>
DNodes all are ListNode so that should make those fine
<headius>
and zarray is an array
<enebo[m]>
yeah that was likely a typo which didn't matter until this change
<enebo[m]>
and tbh ListNode is for common list behavior but ArrayNode is a Ruby concept and it is easy to forget
<enebo[m]>
with that said a, b, c = []
<enebo[m]>
hmm
<enebo[m]>
mri31 -e 'a, b = []; p a, b'
<enebo[m]>
HAHAHA
<enebo[m]>
This single assignment to mlhs is really odd
<enebo[m]>
I just don't think this happens very much
<headius>
yeah assigning [] is basically like assigning nil
<enebo[m]>
but fwiw making ZArrayNode an ArrayNode is a fix of sorts
<enebo[m]>
well it will splat to nothing but the weirdness is it is not how it reads
<headius>
it is a pretty specific case so I think it's an ok fix
<headius>
there's nothing else that's an array but not an array
<enebo[m]>
Someone could say it reads as two variables being assigned an array but then you put a '1' there and that falls apart visually as you are not assigning both to 1
<headius>
I don't see anything else ILiteralNode that should be a problem
<headius>
and ZArray < Array does fix this case
<enebo[m]>
This is sort of the fun and horrible part of Ruby. The language syntax is really unregular but you mostly only notice it when you do something no one would normally do
<enebo[m]>
so you mentioned this last week that somehow this is not always the same result
<headius>
yeah I have not dug deeper
<headius>
it may be a bad test but I didn't see an obvious problem
<enebo[m]>
Seems like a weird error
<enebo[m]>
not supported does not seem like it can possibly be the actual error
<enebo[m]>
I think you saw in another fix I did I had to reset errno to 0
<headius>
right
<headius>
yeah this could easily be that
<headius>
and randomly picking up some other errno depending on test order
<enebo[m]>
so multiple tests in flight with differing orders and we hit something unsupported then this test
<enebo[m]>
which if so could be a significant problem to those unlucky enough to reproduce that order
<headius>
right
<enebo[m]>
something also feels wrong that we have to reset errno
<enebo[m]>
ok this should be simple to repro if this is the case. we just need to call something which does unsupported and then which ever posix call is happening in that
<enebo[m]>
It literally reports the 'def' line
<headius>
this masgn change seems to have zero effect on performance, which beggars belief
<headius>
yeah that has to be some problem with backtrace
<enebo[m]>
end if have_symlink? and !no_broken_symlink?
<enebo[m]>
Or does it!
<enebo[m]>
hmm
<headius>
oh gross
<enebo[m]>
seriously though...is it really up to callers to reset errno
<headius>
@a = 1 pushes fixnum, then pushes stuff that should be below fixnum on stack and does dup2 and pop tricks to reorder
<enebo[m]>
I can also see why this is a flaky test if it is from this if
<headius>
what lazy dingus wrote this
<enebo[m]>
oh also remember I never fixed ordered execution in masgn :)
<headius>
oh I mentioned that to Jeremy and he laughed
<headius>
he said that was a pain to untangle for practically no value
<enebo[m]>
It has nothing to do with this bug but still
<headius>
or something along those lines
<enebo[m]>
yeah the code which will hit it would be very odd and have to be new
<enebo[m]>
because old code would work for an order which changed
<enebo[m]>
It is not super hard but it is involved because you have to make an extra datastruture to save up all the assignments
<enebo[m]>
mostly due to masgn being nestable with more masgns
<enebo[m]>
but those all call off of the same rhs array
<headius>
yeah
<headius>
I'm not too worried about it
<enebo[m]>
me either :)
<enebo[m]>
but as it is a language feature I feel it gets more importance even if totally unimportant
<enebo[m]>
Here is my thoughts on this MRI test file...being lazy at a fault
<enebo[m]>
It will not see if it can hard_link? or soft_link? etc... because tests which care about that may not get called
<enebo[m]>
but then it sets a class variable and does it all lazily
<enebo[m]>
to avoid a small collection of small system calls
<headius>
so this dup2; pop; thing is only in non-indy bytecode
<headius>
I'm not spending a lot of time making non-indy bytecode smaller because my hands are kinda tied there
<enebo[m]>
so asm is helping you? :)
<headius>
helping me?
<headius>
I could probably improve this by assigning a local but it does this because it needs to get the accessor from self and then also assign to self without reevaluating it
<enebo[m]>
dup2/pop is explicit
<headius>
it is a bit odd though... is the receiver operand for ivar assignment ever not self?
<enebo[m]>
compiler.adapter.dup2(); // self, value, self, value
<enebo[m]>
headius: but you can just check whether it is on self or os the issue indirection of self?
<enebo[m]>
like we are passing ni %t_12 and not %self
<headius>
every place we construct PutField it passes buildSelf()
<headius>
which does scope.getSelf()
<enebo[m]>
yeah so in JVMVisitor you can just compare and see if it is self
<headius>
so it's literally the Self singleton
<enebo[m]>
then call the better version
<enebo[m]>
which you will have to make :)
<headius>
phooey
<headius>
how about I just error if it's never self and make the existing version the simpler version
<enebo[m]>
I mean I could add a PutSelfField which extends PutSelf but all that really does is maybe add a simple visitor method
<enebo[m]>
err PutSelfField extends PutField
<enebo[m]>
Oh well it may error at some point
<enebo[m]>
If the inliner is ever re-enabled it may retarget that to be a specific self
<headius>
aha that makes sense
<headius>
it is for inlining
<enebo[m]>
yeah
<headius>
perhaps I should check if it is copyable and then it's ok to just reevaluate it
<enebo[m]>
I don't grok that statement
<headius>
any inlined self would be in a temp var too
<headius>
which is safe to evaluate again
<headius>
perhaps I'm not using the right check? Not canCopyPropagate?
<enebo[m]>
can copy propagate is going to just be immediates in the language
<headius>
and variables no?
<headius>
looks that way
<enebo[m]>
we know 9 can just be used directly
<enebo[m]>
variables are pointers or sorts so you can reuse that reference
<enebo[m]>
I was not aware it was copypropagate though
<enebo[m]>
but I think all variables are copypropagate in the sense they are just opague holders of something. The only issue is I think it can only do that for reads
<enebo[m]>
but nothing is copy propped as a write already
<headius>
right
<headius>
I think this is ok
<enebo[m]>
headius: can you spend a minute to talk about the PR I put above
<enebo[m]>
we sort of have to decide that right now but I have no idea if it is risky or not
<headius>
not too risky
<headius>
mostly it would bring LoongArch and some aarch64 fixes o 9.3
<headius>
should I take a closer look at the changes that would come in?
<headius>
I'm doing this putfield/getfield change just because
<headius>
if it's not copy proppable I reject compilation, but it should still work for all known cases
<enebo[m]>
headius: yeah if we want this in 9.3.10 we should make sure nothing with much risk is in it but there is a motivation to udpate so?
<headius>
well, aarch64 problems are breaking puma on M1 I think
<headius>
newer puma hitting something that we don't do right
<headius>
I haven't followed the issue closely
<enebo[m]>
ok so a M1/2 issue with puma is pretty decent motivation
<headius>
ok making zarray < array broke a bunch of stuff
<headius>
I will just add a case for it in masgn build
<enebo[m]>
interesting on error
<enebo[m]>
looks like we don't handle something as ArrayNode right in the masgn compilation code but it is hard to imagine what
<headius>
I re-pushed with a more localized change
<headius>
not sure what's up with all that breakage
<headius>
now I am baffled why my ivar change is breaking everything
<enebo[m]>
case ARRAYNODE:
<enebo[m]>
if you look at that error we look at NodeType and not instanceof
<enebo[m]>
this should just add an case ZARRAYNODE
<enebo[m]>
it casts to listnode
<headius>
hmm
<enebo[m]>
the only weird bit here is I am unsure is perhaps this will leak around elsewhere
<headius>
yeah
<enebo[m]>
That is odd though
<headius>
I'm spooked now
<enebo[m]>
since it means a call is now getting a Zarraynode
<enebo[m]>
totally different area though
<enebo[m]>
this is not from masgn at all
<enebo[m]>
I do see failures in yield so this is also affecting the same code (or at least it is shared) as generic masgn
<headius>
I found my ivar problem
<headius>
pushed to optz branch
subbu has joined #jruby
subbu has quit [Ping timeout: 252 seconds]
<headius>
enebo: does IRB work for you right now on master?
<enebo[m]>
Let me check
<enebo[m]>
LoadError: no such file to load -- timeout
<enebo[m]>
That is a weird error :)
<headius>
that's because you need a clean build to install the gem
<enebo[m]>
wot
<headius>
it happens when you use the same local repo for 9.3 and master
<headius>
because timeout is versioned on 9.3
<enebo[m]>
I do not
<headius>
hmm
<enebo[m]>
but I did bisect way back last week
<headius>
that would do it
<enebo[m]>
maybe too far back?
<headius>
only switched to timeout gem this fall
<enebo[m]>
how do you clean up your env?
<enebo[m]>
A full rebuild and I was able to run irb
<enebo[m]>
ah but I did merge a float rounding PR...I will compile that in...no way that causes issues
<headius>
hmm ok
<headius>
I get a weird error
<headius>
on master too
<headius>
I might have something weird installed that's messing with it...newer IRB or bad reline or something
headius has quit [*.net *.split]
enebo[m] has quit [*.net *.split]
headius has joined #jruby
<headius>
yes
<headius>
I can test that branch too
<headius>
branch works
enebo[m] has joined #jruby
<enebo[m]>
It will not matter presumably for 9.3 since we are not shipping newer irb with it but it is just another issue away when something hits that file
<headius>
testing previous commit and then we'll know for sure
<headius>
yeah exactly
<enebo[m]>
it is amazing how many jruby-complete problems surface because we try and make this easy to use jar file
<headius>
hmm commit before jnr also works
<headius>
now I am confused
<enebo[m]>
after that last issue I was thinking "I will fix jruby-complete real good by removing it"
subbu has joined #jruby
<headius>
I'm very confused
<enebo[m]>
so it didn't work. then it did work. then you went back and it worked?
<enebo[m]>
Is that the sum of it?
<headius>
yeah
<enebo[m]>
my windows fix made it work
<enebo[m]>
:)
<headius>
I'm trying with jnr update merged to master
<enebo[m]>
Perhaps you had an experimental gem build of soemthing
<headius>
nope still not working
<enebo[m]>
haha wait...it back to not working
<headius>
it does not work on master or 9.3
<enebo[m]>
and seemingly a mac thing only?
<headius>
it works on jnrupdate branch from ahorek
<headius>
any commit I try
<headius>
merge jnrupdate to master, doesn't work
<enebo[m]>
merge it from orbit
<enebo[m]>
oh
<enebo[m]>
ok that is strange
<enebo[m]>
unless his PR was a few commits old and one of those commits caused this
<enebo[m]>
I did update joni on both 9.3 and 9.4 and jcodings on 9.3
<headius>
jnrupdate is based on e9d29fa89ba0053ccadcbfb427aa9ca050a49666
<enebo[m]>
well that is a new joni
<enebo[m]>
but only that on master
<enebo[m]>
err on 9.3
<enebo[m]>
Let me build 9.3
<headius>
ok that commit does work
<headius>
the one I posted
<enebo[m]>
ok well not surprising but irb works fine on head of 9.3 on linux
<enebo[m]>
that is 2 commits ahead of the update commit jnrupdate is based on
<enebo[m]>
but 1 is windows rename of win32api.rb and the other is updated joni/jcoding
<enebo[m]>
headius: I don't remember... what is the error?
<enebo[m]>
or I don't see it above
<enebo[m]>
headius: once this is resolved it would be good for you to verify puma is working on 9.3 as well