genpaku has quit [Remote host closed the connection]
genpaku has joined #jruby
genpaku has quit [Quit: leaving]
genpaku has joined #jruby
<headius>
oh awesome
<headius>
I forgot to triage this hang yesterday
<headius>
oh this is in a spec, interesting
<headius>
master seems to be hanging in MRI tests
<headius>
$ jdeps lib/jruby.jar
<headius>
jruby.jar -> java.base
<headius>
seems like the module name got lost somehow
<headius>
aha the MRI test hang is when I merged in updates for test_rubyoptions, so that's probably got a hanging case on CI
<headius>
enebo: I have not triaged bugs but I'm up early here to do that and get the first stages of this joni thing landed
<headius>
ugh, GHA won't show output until it times out
<headius>
guess I'll have some breakfast
<kares[m]>
Hey, been playing with error-prone compiler recently.
<kares[m]>
Was wondering, given it slows down compilation if I always enable the extra checks, should error-prone be enabled behind a maven profile and run on CI by default?
<kares[m]>
Seems it's able to find some bugs in the JRuby code-base (nothing super critical so far), will post them in a PR.
<headius>
yeah could be useful
<kares[m]>
okay, will set it up behind a profile, for now - than you guys could tell how much of a slow-down you notice
<headius>
enebo: I have trimmed 9.4.1.0 issues and we can discuss the rest. My PRs can be merged but I wanted to re-run CI after fixing this MRI core test hang (to verify they are ok)
<headius>
the random PR is especially important for 9.4.1
<headius>
joni needs a release and I lost access to my release machine again, but once that happens we can update and merge the joni field accessor PR
<headius>
that is necessary to get a release of strscan out to bridge from old way to new way
<headius>
kares: need to merge or delay your PRs marked for 9.4.1.0
<headius>
we want to release this week, possibly today
<kares[m]>
will check if there's anything risky
<kares[m]>
is 9.3 still merged back to master?
<kares[m]>
oh yeah it was 4 days ago ... okay that helps
<enebo[m]>
kares: #7571. Have you reviewed how MRI does this? I would like to completely deprecate ID from Warn
<enebo[m]>
I did change ones I noticed and most just end up being 1 or 2 param things which build up a string (although I have little doubt there are full string deprecation warnings in MRI I just don't know the method
<enebo[m]>
rb_warn_deprecated("lambda without a literal block", "the proc without lambda");
<enebo[m]>
public void warnDeprecatedAlternate(String name, String alternate) {
<enebo[m]>
I believe this should behave the same
<kares[m]>
enebo: nope I did not - just wanted smt less confusing as a "message" argument was used where a `name` was expected
<enebo[m]>
yeah
<kares[m]>
btw. those IDs might be useful if there was a way to set a funtion by user-code to filter out specific IDs ... but than yeah not sure how much useful it would be in general
<enebo[m]>
this need not get fixed for 9.4.1.0 but eventually hopefully our warn calls just matchup with MRI (sans their C-like names)
<kares[m]>
should I revert than, since I already merged this morning?
<enebo[m]>
kares: yeah they were put there for that purpose to begin with but many IDs are nonsense now so I am not sure if anyone ever used them
<enebo[m]>
Your fix will remove some weird extra text so I think you behaviorally fixed an issue
<enebo[m]>
and it will take a little bit of time to verify that Alternate method is doing the right thing too (note: I added these but some of this work was done pretty quickly)
<enebo[m]>
Another issue which I think headius observed late last week is in some warn methods we are building up a string and then not using it because warns are enabled
<enebo[m]>
This has nothing to do with this PR but it underscores I think why MRI also have just the name or name+alternate as params vs a full string
<enebo[m]>
I should say he only noticed one of those but I bet there are more
<headius>
Yo
<enebo[m]>
has final ever did anything in the JVM?
<enebo[m]>
as far as optimization? I am pretty sure it didn't in the past
<enebo[m]>
kares: the error-prone things it found largely look good even the generics changes BUT public API changes which enforce generics will change API at a compilation level (but will still work fine against older stuff at runtime)
<headius>
enebo: I gained access to my work machine again so I'm releasing joni 2.1.48
<headius>
2.1.47 contained the field accessors but then I realized we need to deprecate the Region constructors if we want to split it into specialized shapes, so 2.1.48 adds a factory method
<headius>
2.2 will privatize the fields and remove the constructors
<enebo[m]>
yeah I saw that somewhere this weekend
<headius>
I'd love to know what potato Sonatype runs the OSS maven stuff on because it is so slow sometimes
<headius>
ahh darn, the test run of mri:core did output progress but it does not show which test hung
<headius>
maybe I just revert the excludes for now and revisit
<enebo[m]>
headius: yeah I figured why not wait. I think it is fine but not a big deal
<headius>
wait on 5.1?
<enebo[m]>
!=
<headius>
oh sure
<kares[m]>
<enebo[m]> "kares: the error-prone things it..." <- thought the same but was pursued (by someone else) that for the JavaEmbedUtils <T> changes it should be no problem, due the way they were returning Object
<kares[m]>
is that still backwards incompatible, in terms of compilation?
<enebo[m]>
kares: if an existing piece of code can still compile against the signature then great
<headius>
should not be incompatible
<enebo[m]>
I know it will still run
<headius>
that erases to Object
<enebo[m]>
during compilation?
<headius>
yes
<headius>
oh well during compilation it will just be Object with a cast now
<enebo[m]>
Oh so Vector foo = somethingNowGeneric is fine
<headius>
so the only change is the autocast to T
<kares[m]>
I can add explicit tests on the Java side, thought there were some ...
<enebo[m]>
I understand it is erased at runtime but I did not remember it will not care about generics at compile time for return values
<headius>
not that it doesn't care, but it won't affect the end bytecode nor break any existing compile that just does Object o =
<headius>
T will just be Object in that case
<enebo[m]>
yeah I specified I was not concerned about the runtime part of this
<headius>
so no cast gets introduced
<enebo[m]>
I know the class format is erased
<enebo[m]>
I am just worried an existing API which decides to compile against the latest JRuby will have to change their source to compile against it
<headius>
yeah and I'm saying it won't matter at compile time either, because existing code that just does Object o = ... will just have T = Object and work as they do now
<enebo[m]>
ok so long as no one has to change their source to continue compiling I am fine
<headius>
and that's all they could do before because it was explicitly Object, so it's fine
<headius>
joni release is making it's ay into the world now and I pinged hsbt about releasing psych 5.1
<headius>
we'll need a separate PR for master to use it because the 9.3 one has failures and shouldn't be merged (failures because the tests and specs are based on older psych)
<enebo[m]>
heh. I believed you both but at the same time I had to write a program to prove the erasure is at compile time
<kares[m]>
will do that - add a few test-cases that are based on actual JavaEmbedUtils API usage
<enebo[m]>
It is weird that it just is not what I remember and that would definitely be a false memory
<kares[m]>
nope it's certainly an issue in some cases, but not this one
<enebo[m]>
kares: it won't hurt to have more tests showing how to use it
<enebo[m]>
kares: and I usually dislike the error finding tools because they have a lot of false noise but that one had only good stuff in it
<enebo[m]>
I am not against using the tools in general but just generally not as part of CI
<enebo[m]>
If this one has the quality of the run you did then it looks like a good idea
<enebo[m]>
headius: so 5.1 is using the newer engine artifact right?
<headius>
yes
<headius>
if we want to "fix" that CVE in 9.4.1 we need this
<kares[m]>
enebo[m]: yeah, wanted to give error-prone a go and indeed it seems to be finding useful stuff.
<kares[m]>
will do some 'compilation time' numbers and leave them on the PR (once finished) ...
<enebo[m]>
headius: I wanted it in so we could give time to test for 9.3.11.0
<headius>
right that too
<enebo[m]>
I don't care about the CVE in the short term since it is not real and I don't think people will not use 9.4.1 because of that as much as it is very early in point releases
<enebo[m]>
I do care about it in 9.3 but it was so fresh I did not think it was worth putting in that release yet
<enebo[m]>
having said that if we miss the boat f0r 9.4.1.0 we will have it on both branches as soon as it is released so perhaps it isn't a big deal (from CVE perspective)
<enebo[m]>
9.3.11.0 will still take same amount of time to get released and hopefully enough time to feel confident there isn't something hidden using the new engine
<enebo[m]>
headius: another question though...does 5.1 have some other fixes we do care about?
<headius>
for my part, only the exposure of some SnakeYAML config options in parse
<headius>
there are probably other things fixed by not-me
<headius>
I'm waiting for joni to propagate before updating the region field PR with released version
<enebo[m]>
oh yeah so donv can fix his issue too
<headius>
right
<headius>
there's no equivalent API in the libyaml ext but only JRuby peeps want this right now
<headius>
csv_optz and random_random PRs are rebased and rerunning
<headius>
jit_optz predated the hanging tests and was green so I already merged that
<enebo[m]>
headius: yeah that PR I think is creating a problem while solving another
<headius>
I was wondering about that
<enebo[m]>
I have been pondering it but it will just ping-pong the symbols encoding if it is changing or worse stop matching if an errant one comes in
<headius>
since we can't key off anything to support two encodings for the same symbol
<headius>
it appears to change encoding of the existing symbol
<enebo[m]>
So long as the bytes are the only thing considered perhaps the matching would still work?
<headius>
right
<enebo[m]>
yeah it just toggles to last one wins
<enebo[m]>
we would like to ultimately have both win
<headius>
so it doesn't improve anything really
<enebo[m]>
yeah I think it will remove some reports but then cause even more confusing reports
<headius>
I will comment
<enebo[m]>
the only caveat is not many people report this as a problem to begin with
<enebo[m]>
I should have but I was thinking about how bad it would be in practice and I guess the thought of seeing someone report a mysterious send which stopped working would lead back to this and make me angry :)
<enebo[m]>
Marshal encodes info as a byte in a regristration map
<enebo[m]>
I thought about all encodings just being a byte at front or back which is ignored past identity
<headius>
yeah I'd rather wait and see if anyone is affected by not fixing it
<headius>
so far nobody has reported it
<enebo[m]>
yeah thus far it is just knowing it is broken which bothers us :)
<headius>
I also thought about embedding the encoding in the key strings but then there's no way to look it up without encoding
<enebo[m]>
It would be very weird to have to iso_8859_x which both are different symbols but the same bytes which are used for a send or something
<headius>
which is maybe ok? not sure
<headius>
enebo: and in that case it wouldn't matter either since we only key method tables off the bytes too
<enebo[m]>
well it would matter if there were two methods and in MRI they would go to different methods
<headius>
you can't have the same method with same bytes in name and different encodings right now anyway
<enebo[m]>
we would only go to first one entered
<headius>
more than that, we will only have one method entry
<headius>
this is where the embedded encoding gets really sticky because you couldn't just callMethod("foo") it would have to be callMethod("UTF-8:foo")
<enebo[m]>
I am talking about Ruby compat and not about how we do things.
<headius>
yeah for compat we'd need to key symbol table off encoding as well
<headius>
but we can't support the method table part without big changes
<headius>
i.e. eliminating String as key
<enebo[m]>
I was just pointing out one scenario to show how unusual it would be to see it happen in real life
<enebo[m]>
I was going to start talking about this internally but we should just stop :)
<enebo[m]>
It is annoying but not important
<headius>
yeah even the original case that forced us to add encoding was peculiar, source files encoded in some JP format and the symbol names were not
<enebo[m]>
It like feeling bad because flip-flop not being supported hurts our spec percentages :)
<headius>
but the JP formats are not compatible with UTF-8 anyway and will rarely overlap with US-ASCII
<enebo[m]>
you would hope not
<enebo[m]>
but it is so obscure I think people would just work around it anyways
<headius>
hey we could implement flip-flop any time
<enebo[m]>
but I still doubt nayone has really hit it either
<enebo[m]>
just revert what we had removed
<enebo[m]>
AST still parses it
<enebo[m]>
we just do not build it and throw an exception
<enebo[m]>
but there was some impl problem which lead to us nuking it from orbit
<enebo[m]>
headius: you landed your series of small opts bytecode changes. Did you see any improvement beyond parser getting closre to compile
<enebo[m]>
It is possible we talked about this the other week but I don't remember
<headius>
I did not measure much else... functionally most of this is zero-sum just with smaller bytecode
<headius>
there are a few things that gained indy versions to shrink down
<headius>
those might improve perf slightly but they weren't drastic changes
<enebo[m]>
yeah I always wonder about smaller bytecode just because they seem to do a rough count for some opt decisions
<enebo[m]>
rough count of what? something
<headius>
right, it's sorta silly but my goal was more toward getting larger methods to compile
<headius>
smaller bytecode in general will be good for inlining though
<enebo[m]>
yeah it makes sense and it might make a difference too if something small getting smaller hits some threshold
<headius>
there's more to do but I figured we could get this round of improvements released
<headius>
oh we did talk about one other change I did not make: bump up max IR size for JIT
<headius>
3000 allows one of the two lexer methods in parser to compile, and it does also native JIT at that size
<headius>
I proposed 5000 but have no method to test that threshold with
<headius>
current is 1000 and clearly it is too small if a method from 3000 IR instructions still native JITs
<headius>
(granted not all IR are the same and may produce more or less bytecode)
<headius>
enebo: I think I'm going to make the WIP jobs always return 0 so they don't show up as failing
<headius>
or we could remove them
<enebo[m]>
hmm
<headius>
not sure why I did not make them always pass in the first place TBH
<enebo[m]>
sort of seems like we want to know it is too big but also works and perhaps not worry about a limit other than perhaps one we know will fail
<enebo[m]>
being too big to compile means wasting CPU so we want the value to be close but I think we would rather have a few too big methods than more smaller ones which could have
<headius>
I have not re-checked size of that parser method after all of the jit optz either so it may be a bit smaller
<headius>
csv optz looks green
<headius>
that only had two small things: kill off an error StringBuilder being constructed for every read of encoded IO stream, and preallocate those Ptr objects once per IO since they're used under lock anyway
<headius>
the former fix probably affects lots of users, though unclear how big an impact it has
<headius>
for lots of small reads it could be big
<enebo[m]>
yeah I don't see any need to land that this morning but it seems safe so I guess I don't see a risk to it either
<headius>
might as well get it in the wild
<headius>
enebo: so what say you about bumping IR max
<headius>
we can do 3000 to be safe and see how it goes
<enebo[m]>
it won't fail to run just will report and error right?
<enebo[m]>
and burn some user cpu trying
<headius>
won't even report error, JIT will just fail and we fall back on full
<headius>
will report error with -Xjit.logging
<enebo[m]>
yeah that was what I meant
<enebo[m]>
yeah seems fine. As you said you saw nothing fail to jit at 3000
<headius>
ok
<headius>
I'll just push that to master like a boss
<headius>
I also notice we still have a 10k total jitted methods limit in place right now, we may want to revisit that
<headius>
not today
<headius>
I'm not sure I'll be able to get psych 5.1 out today... it is already night time in Japan and I need to get a thumbs up from hsbt at least (if not getting him to release it)
<headius>
I should have released over the weekend, my bad
<headius>
too many projects on the stack
<enebo[m]>
yeah so we can go without I think
<enebo[m]>
sometimes not everything makes it is
<enebo[m]>
in
<enebo[m]>
brb
<headius>
the random fix is finally green too, merging
<headius>
only failure in region_fields was from broken random (plus two jobs on M1 that failed to see joni 2.1.48 in maven), I'll merge
<headius>
I'm going to close the StringIO#initialize encoding: thing because it's no longer in this codebase and I started a PR to fix it in ruby/stringio
<enebo[m]>
ok
<headius>
down to 7 open items
<headius>
I need to push a PR to test psych 5.1 on master but that may not get released today
<enebo[m]>
I moved my kwargs stuff to next release since it is not ready
<headius>
ok
<enebo[m]>
#7607 is fixed right?
<enebo[m]>
The other two should just punt to .2
<enebo[m]>
kares has one open PR
<enebo[m]>
and you have one for openstruct
<enebo[m]>
kares: your new generics will not break existing callers so I am fine with this landing and it can be cp'd back to 9.3 too
<kares[m]>
okay - just pushed final touches a minute ago ... will merge in a bit as CI picks up?
<enebo[m]>
ok
<headius>
we can kill some merge commit CI jobs to speed this up
<headius>
this needs to be updated to psych 5.1.0 (not pre1) before merge but there it is
<enebo[m]>
This sounds unlikely to get released today so what should we do? Release today but omit psych or your work in some time next day or two for a release then?
<headius>
I retargeted the two remaining actual bugs, one about readline not resetting TTY and the other about closing a failed Socket raising EBADF
<headius>
all bugs related to Psych update fixes are targeted to 9.4.1.0
<headius>
I think we wait a day because otherwise that guy will continue complaining about jruby-complete
<headius>
even though we can release a "good" psych after 9.4.1.0 it won't be in the complete jar
<enebo[m]>
ok. I personally don't care if he complains another month but if you have the time I am fine with this
<enebo[m]>
we should coordinate a time to check in tomorrow for this since we are 7 hours apart
<headius>
yeah it's fine... morning US time will be right after my talk so I can focus on release tasks
<enebo[m]>
ah sweet
<headius>
speaker dinner is also tonight so I will have to run shortly
<headius>
"have to"
<enebo[m]>
ok. hey might as well.
<headius>
I think we're just waiting on kares PR to go green and psych 5.1 release at this point
<enebo[m]>
yeah
<headius>
I killed off a couple meaningless CI jobs.. jit maxSize=3000 job, kares PR, and psych 5.1 PR in the queue
<headius>
ok ttfn, I'll check back after dinner
<headius>
or during if it is boring
<enebo[m]>
lol
<kares[m]>
merged
<kares[m]>
3000!
<headius>
Feel the power!
<headius>
FWIW I have seen very few methods too large to jit