<headius>
Doing well! Back to serious work tomorrow
JasonLunn[m] has quit [*.net *.split]
distant[m] has quit [*.net *.split]
Freaky has quit [*.net *.split]
Freaky has joined #jruby
Freaky has joined #jruby
Freaky has quit [Signing in (Freaky)]
distant[m] has joined #jruby
JasonLunn[m] has joined #jruby
<puritylake[m]>
Been on holiday?
<puritylake[m]>
I’ll be back to work on JRuby either today or tomorrow. Possibly squashing bugs to start out with
<headius>
Yeah catching up some family trips we missed the last few years
<puritylake[m]>
Ah nice, sounds like a good time, I had noticed the chat was fairly empty
<headius>
Yeah time to get back in there!
<puritylake[m]>
Enebo still kicking around, or did he also take holiday time?
<headius>
I think he took some breaks here and there but not sure of his status
<enebo[m]>
I have been working but just mostly in silent mode
<enebo[m]>
I landed new kwargs using out of band callInfo on TC
<headius>
nice
<headius>
I am catching up issues to get back into the swing of things
<enebo[m]>
I definitely think we may want to consider pushing some kwarg call paths so we can get away from that method
<enebo[m]>
k77chi just reported a potentially decent thing with timeout
<headius>
I agree, it needs to happen... for 9.4 or after?
<enebo[m]>
He said in last sentence we will get more green
<headius>
yeah I saw that
<headius>
have not looked at the issue yet
<enebo[m]>
I think it is to be determined on kwargs. I should probably even write something up and what is good and bad on how it works now
<enebo[m]>
puritylake: on other thing I was working on was implementing grisu2 for float/double to string
<enebo[m]>
It is implemented but some calculation is drastically wrong so I need to debug it
<headius>
performance wise it will have an impact but we already go to TC for a lot of stuff
<headius>
the other aspect is having state bugs if some logic fails to update TC properly
<puritylake[m]>
Ah interesting, I was kinda dreading getting those float/double to string code in lol
<headius>
passing the info along call path is clearly the way forward, it's just a lot of duplicated signatures to update
<enebo[m]>
yeah I mean it hits a field and reads it and then resets it. So math benchmarks? maybe
<headius>
yeah it is just memory reads and writes
<headius>
on same thread
<headius>
we know there's a cost updating frames and such but it is small if there's any other heavy lifting (e.g. allocation) going on
<enebo[m]>
seriously I think the bigger problem is that it is really brittle
<headius>
and until we can splat out kwargs on both ends and avoid the hash object it will probably be miniscule
<headius>
hash is huge overhead in comparison
<enebo[m]>
If you inject any "ruby" call between setting and reading that other call overwrites it
<headius>
yeah so it has to happen purely in the substrate between caller and callee
<enebo[m]>
So a few places we capture the value knowing it is likely to do some coercion (e.g. to_hash) and then consume it or reset the value
<headius>
that is the kind of state change but I was referring to
<headius>
bug
<enebo[m]>
which is a big reason it should just be a param down the call path
<headius>
hah yeah scary
<headius>
make it a stack
<headius>
loljk
<enebo[m]>
I also conditionally added "forward=true" which preserves consuming it in native methods which plan on forwarding the callinfo to another method like Class#new is really just forwarding to initialize
<enebo[m]>
In truth all methods have to handle some_method(real_arg, **{}) even when they don't support kwargs
<headius>
ugh I can never remember some IR class names
<enebo[m]>
in Ruby to Ruby calls this is all handled by instructions (and ruby callsites just omit passing empty kwargs
<headius>
set or put or assign
<headius>
PutConstant today
<enebo[m]>
I would be happy if they were literally the name of the class
<enebo[m]>
I am not attached to pretty names
<headius>
so my opinion on adding the call paths is that the perf gain is not a driving factor until we can propagate kwargs everywhere, which is probably too big for 9.4
<headius>
so it would center around having more robust kwarg state handling
<enebo[m]>
yeah I am in the same boat
<enebo[m]>
I think it is the right step but it is a massive undertaking
<enebo[m]>
I think we limp this design along and then get 9.4 out
<puritylake[m]>
Need any help on my side? I’m happy to go bug squashing until 9.4 releases if needs be
<enebo[m]>
puritylake: any fixes of anything not passing on master/9.4 is a big help
<enebo[m]>
for the grisu2 impl I noticed there are other Java impls so if I cannot figure out which math is wrong I will just run those and see how my math differs from theirs.
<enebo[m]>
This is all annoying because all the impls come from the same paper but they all have different licenses
<headius>
enebo: you have any issues where generated pom.xml have element ordering changes?
<enebo[m]>
ummm
<headius>
I know we have seen this from time to time but I'm running on Java 17 locally and they are always showing as dirty after a build now
<enebo[m]>
I don't really grok that. Perhaps unrelated but I can never load pom.rb in idea for core unless I delete it and depend on the pom.xml instead
<headius>
pretty sure this is just polyglot not using a specific order and letting hash order happen
<enebo[m]>
I have not noticed that but I don't really regenerate those any more than version numbers so I may not notice it
<headius>
they just generate on build so it is annoying
<enebo[m]>
updating VERSION will regenerate but they are already changing the files
<enebo[m]>
looks like some hash weirdness
<headius>
add it to the queue of things to fix
<enebo[m]>
don't we have an option to disable random hashing?
<headius>
I don't know if it would matter... I think this is just differences in how e.g. Hashtable hash in newer versions of Java so iteration encounters keys in a new order
<headius>
and polyglot just uses the order the hash/map gives
<headius>
it might even be fixed in newer polyglot, have not looked into that
<enebo[m]>
so we don't control the hash used either
<enebo[m]>
but this only fires if the .rb changes?
<enebo[m]>
Or you said for all builds but I wonder why?
<enebo[m]>
I guess maybe to avoid just depending on dates
<headius>
it generates every time
<headius>
I think the maven builds still depend on the xml so polyglot always generates it
<headius>
maven itself depends on the xml I mean
<headius>
there's no direct hand-off of the model from polyglot to maven
<enebo[m]>
yeah so no date checks to know if it needs it probably because that is not 100%
<enebo[m]>
well it is 100% unless you have a weird environment
<headius>
yeah
<enebo[m]>
anyways I guess that is not really a big issue since 17 is quite a ways out from being a lowest dev build but annoying if you accidentally start commiting that sutff :)
<enebo[m]>
going to go for a run this morning...70F!
<enebo[m]>
It will be hot as balls the rest of the week
<headius>
yeah storms cooled things down a bit
<headius>
I will keep reviewing bugs, fixing some, and catch up
subbu has joined #jruby
<headius>
enebo: this is clearly an IR bug... the fast path for a rescue body with no return logic is to copy the exception into the return value for the begin/rescue, but that clearly is wrong when it should go into a specific variable:
<headius>
we could try nulling it out in that case but I'm not sure what else below might be expecting the exception to be in `rv`
<headius>
removing the Copy does not work... breaks something in RG during boot so it is probably not the right fix
<headius>
this is very old code so this probably has always been broken
<enebo[m]>
yeah
<enebo[m]>
I will take a look too. I saw this come in but I was trying to finish off landing kwargs branch
<enebo[m]>
hahah may
<enebo[m]>
well perhaps it came in earlier than that
<headius>
It's probably simple but my knowledge of the rescue building logic is a little fuzzy
<enebo[m]>
oh yeah this was reported like a few days after our last release
<headius>
Notice it only affects the case where you have an empty rescue and you are also assigning the exception to a variable
<headius>
Perhaps the compile of the body should not be returning that variable when there's nothing else in the body?
<enebo[m]>
yeah this is interesting because I think you could argue it is not empty
<enebo[m]>
So that might be the fix
<headius>
Any assignment done for the exception variable should not be considered part of the body, at least for return value purposes
<headius>
The result of assigning that variable should just be dropped
<enebo[m]>
The assignment to _e should perhaps reflect it is not an empty body assuming that is simple
<headius>
I think you're right
<headius>
It sees that there's a variable assignment and treats it as a non-empty body with the result of the variable assignment as the return value
<headius>
I'm not sure how to fix that since the body probably does have the variable assignment as a normal instruction. It just shouldn't be considered the return value of the body
<enebo[m]>
so opposite of what I said. It is empty but it thinks the assignment is part of the body
<headius>
Right
<enebo[m]>
in my mind it is but the => assignment is .... hmm
<enebo[m]>
This is a generic thing now in Ruby too
<headius>
So it is not really a bug in the rescue logic, it is a bug in determining whether the body has a returnable result
<enebo[m]>
what does p("foo" => e) do?
<enebo[m]>
I wonder if that even returns "foo" or not
<enebo[m]>
lol
<enebo[m]>
hmm
<enebo[m]>
It thinks it is a hash now :)
<headius>
I think this just needs to recognize that the body starts after the assignments and check whether that is empty or not
<enebo[m]>
This may also be a bit academic since this is a Ruby 2.6 bug and earlier and the generic form of rhs assignment is a newer feature
<headius>
The variable still needs to be assigned but it should not be considered the last instruction in the body for return purposes
<enebo[m]>
Now I want to know how that works though
<enebo[m]>
It will be the same as the more generalized feature
<headius>
It's just an assignment
<enebo[m]>
so as an expression it returns the value
<enebo[m]>
wow intellij crashed
<headius>
Well done
<headius>
The assignment is really part of the preamble to the body
<headius>
Like assigning local variables from method parameters
<headius>
The assignment should not be considered part of the expression that is the rescue body
<headius>
enebo: always put "fixes" in the body of the commit and it will link it in the commit and PR
<headius>
fix is nice and simple, ship it!
<enebo[m]>
headius: It did link on the actual issue with just subject but I keep forgetting about putting in body of message
<enebo[m]>
then I don't want to edit original commit
<enebo[m]>
hence the windy road I take
<headius>
yeah I never put the bug number in the top line because git also prefers that line be under 60 chars or something
<headius>
so short description of change in top line and long desc + fixes in description
<enebo[m]>
I find it annoying it does not create a link on the subjectl ine for #nnn
<enebo[m]>
I just like to see in git log subjects what it was for
<headius>
yeah that is just a GH limitation I guess, but it is annoying you have to have multiple lines to make it work
<headius>
would be nice to have bug numbers in subject but doesn't seem to be the preferred pattern
<headius>
I guess the idea is that one-liners should not be a thing
<headius>
so might as well keep subject short and specific
<enebo[m]>
but the bug number is still nice outside their website
<enebo[m]>
anyways it is not that important I just wish they would add a link
<enebo[m]>
It links the important way too
<headius>
it will also format commit subjects longer than 60 chars weirdly so I try to always obey that one
<headius>
it = GH
<enebo[m]>
yeah
<enebo[m]>
it does the ... and just continues into the comment
<headius>
doesn't leave a lot of room for "fixes #####"
<enebo[m]>
vim seems to warn me at 60
subbu has joined #jruby
<headius>
that timeout thing is a conundrum
<headius>
it's like the exception is not being raised properly
<enebo[m]>
pushing a merge soon jruby-9.3 -> master
<headius>
No problem
<headius>
The timeout thing seems to be a thread polling issue, the range each logic does not pull during any of the loops
<headius>
So it is kind of a contrived case because nothing happens within the each body to trigger a thread poll but I'm guessing this is copied verbatim from the spec
<headius>
And the each loops should probably be checking after each iteration anyway