drbobbeaty has quit [Remote host closed the connection]
drbobbeaty has joined #jruby
_whitelogger has joined #jruby
<headius>
strscan extra file issue has been fixed and released
<puritylake[m]>
Been back at failing Array tests, made a PR for Array#[]=
<puritylake[m]>
Or aset in the Java impl
<headius>
Nice
<puritylake[m]>
Apparently a mismatch in types raised, RangeError instead of IndexError
<puritylake[m]>
Checked against CRuby 3 and it is meant to be an IndexError even when checkInt raises a RangeError
<puritylake[m]>
So I caught the RangeError and raised an IndexError in its place
<headius>
kares: I noticed while profiling this timeout impl that we appear to be creating a new Metaclass for every time we pass a proc for an interface argument to Java
<headius>
Have we never come up with a more efficient way to do that? Given a set of Java interfaces, we should only have to create a proc delegate class once but there were hundreds of thousands in my small benchmark
<kares[m]>
hmm I thought it was cached on first use ...
<headius>
yeah me too
<headius>
it was executor.schedule(block, ...) with the first argument to be a Runnable
<kares[m]>
might be smt that got broken in 9.3 did you try 9.2?
<headius>
I will try
<headius>
9.2 does not appear to have the same issue
<headius>
I was running master, will check 9.3
<headius>
rank self accum bytes objs bytes objs trace name
<headius>
41643 is live objects and 110481 is total allocated
<enebo[m]>
puritylake: Instead of catching the rangeError can you make a private method on RubyArray checkBeginIndex() which just raises the IndexError directly.
<enebo[m]>
puritylake: We do not really want to make two exceptions per call but this is also an issue in I think slice as well so this will be done more than just this one place
<headius>
enebo: Ruby port of our timeout is moving forward, along with plans to switch the base logic to Evan's impl
<enebo[m]>
I read some of the comments on the issue
<puritylake[m]>
enebo: duly noted, will make that change
<enebo[m]>
puritylake: cool. I think it is just those two but hopefully MRI has tests for slice as well
<enebo[m]>
I guess we are doing it wrong but MRI has not test for this
<enebo[m]>
JRuby does RangeError here and MRI does IndexError
<puritylake[m]>
Yup, which is the problem in the aset method
<enebo[m]>
yeah it may be more than these two but these are the ones I noticed
<headius>
it may have changed at some point
<enebo[m]>
puritylake: this may also be a good opportunity for you to PR for ruby/spec since it is uncaptured behavior
<enebo[m]>
you will expand your OSS reach :)
<puritylake[m]>
Ok sounds good to me
<puritylake[m]>
Now just to see how to go about it lol I assume Ruby has a whole different system to JRuby, PR-wise
<kares[m]>
lots of MetaClass indeed
<puritylake[m]>
Well I’ve been wanting to help out CRuby, just never had the nerve
<puritylake[m]>
Like I’m semi decent at C, but CRuby will have a lot of optimisations and quirks that could take a while to learn, but I guess I could start with helping with Ruby code
<enebo[m]>
puritylake: spec is different in requirements from main c ruby. Technically you could even make a commit to our copy of spec and it will make it there but it probably helps your own visibility to make a PR there directly
<enebo[m]>
Also the experience of hitting other maintainers in how they react to PRs
<enebo[m]>
but you can do either. but just make sure if you go through our copy you keep it in its own commit
<headius>
enebo oh glob
<headius>
these tests run in parallel
<enebo[m]>
GLOB
<headius>
no wonder it is getting bad aliases and unpredictable results
<enebo[m]>
ah
<enebo[m]>
well shoot
<enebo[m]>
I guess we can run serially and make sure it is not messing up
<headius>
these mock/unmock sequences may be incompatible with real concurrency
<enebo[m]>
yeah how could they be :)
<headius>
trying to figure out the incantation to turn parallel off
<enebo[m]>
IF you have n happening at the same time
<headius>
yeah they are not atomic or anything
<headius>
🤦♂️
<enebo[m]>
They would also need to be transactions and not just atomic
<headius>
I could not figure out why they were so unpredictable even with the same seed
<headius>
yeah
<headius>
mock, test, unmock in one atomic op
<enebo[m]>
yeah so they clearly just assume MRI behavior that only one ruby on one thread at a time (and luckily never add anythign which will cause that test to block)
<enebo[m]>
headius: so for our work this week we can either exclude the tests which mock since we have excludes set up in arjdbc
<enebo[m]>
headius: or we can change testing to not run in parallel
<enebo[m]>
So I consistently saw this break withotu worrying abotu seed only running a single test file but that file did multiple of these tests so I guess it was always hitting parallel
<enebo[m]>
AS?
<headius>
just that file entry test
<headius>
but no alias problems
<enebo[m]>
ah
<enebo[m]>
ok
<enebo[m]>
Good I guess
<headius>
PARALLEL_WORKERS=1
<enebo[m]>
Trying all of AS to see
<headius>
yeah me too
<enebo[m]>
It helps to spell parallel with one r
<headius>
haw haw
<enebo[m]>
I was wtf I see those now errors
<enebo[m]>
I believe we may have some additional failures from env setup
<enebo[m]>
like I think we need redis running or something in one of these (I just distantly remember that)
<headius>
yeah
<enebo[m]>
NoMethodError: undefined method `ATTRIBUTE_NODE' for Java::OrgW3cDom::Node:Module
<enebo[m]>
LOL
<enebo[m]>
So crazy...Rails has this JRuby-specific stuff in it
<headius>
not nokogiri?
<enebo[m]>
jdom
<headius>
ah
<enebo[m]>
but this could be very old test code and it is not really used in rails
<enebo[m]>
but no one knows this
<enebo[m]>
and it is never run because we are not in the CI
<enebo[m]>
but earlier Rails unit tests were almost completely green a few years ago so ?
<enebo[m]>
Skipping redis tests. Start redis and try again.
<enebo[m]>
Skipping memcached tests. Start memcached and try again.
<enebo[m]>
headius: well since all three adapters go out at same time I think we need the other two somewhat working
<enebo[m]>
but sqlite3 is not really fully working either...but it is closer
<enebo[m]>
headius: for testing though we can make adapters
<headius>
ok
<enebo[m]>
rake build:adapters - to build all specific adapter gems and the base gem
<headius>
yeah it was a pie in the sky goal to have something users could install after RC
<enebo[m]>
well it could happen still but I am unclear how far we are away and now that we can see results in Rails we need to decide which is more important
<enebo[m]>
71 E/F in AS is not too bad
<enebo[m]>
I see some are cache_store failures and based on their testing I wonder if they are not doing other unrealistic things in those tests using threads
<enebo[m]>
The jdom errors are at least 10 of those too
<enebo[m]>
So we may not be too bad on AS
<enebo[m]>
some issue with constantize
<enebo[m]>
so that is likely needs solving sooner than later
<enebo[m]>
since we know that changed to const_get/const_set
<enebo[m]>
-"argument out of range"
<enebo[m]>
+"Argument out of range: for month: 32"
<headius>
yeah these don't look too bad
<enebo[m]>
lol...we give a better error message
<enebo[m]>
I would not be surprised if we miss most of these errors in a rails app
<enebo[m]>
but the constantize one is a little scary and there is some inheritance one which maybe is depending on subclasses behavior (which is weird if true since that I htought was 3.0+ behavior)
<enebo[m]>
ArgumentError: wrong number of arguments (given 4, expected 3)
<enebo[m]>
a bunch of these so this is something I will examine now
<enebo[m]>
I think this is still possible to see with attr syntax foo[**r]
<headius>
arriving at Oslo by bus now so I will go dark until the flights tomorrow
<enebo[m]>
but this may just be something we have missing a kwargs option
<enebo[m]>
ok
<headius>
waiting on hsbt to review the timeout changes but we can probably get a gem out pretty soon
<headius>
Benoit agrees that the one failing case is a problematic feature that should probably go away
<puritylake[m]>
enebo: just looking to confirm is it slice or splice is the issue?
<enebo[m]>
slice and aset are the two I noticed but MRI internally uses the same method for part of it which just happens to be called splice :)
<enebo[m]>
So just slice and aset are the ones I saw
<enebo[m]>
In other news I found a new kwargs bug in the new impl :)
<puritylake[m]>
In CRuby or JRuby?
<enebo[m]>
in JRuby. This is the stuff I have been working on lately
<enebo[m]>
keywords changed both semantically for users and has internal changes which has forced this to be redesigned
<puritylake[m]>
Ah I see, is there anything I can do to help?
<enebo[m]>
I think just continuing to fix issues is a huge help on our CI failures
<enebo[m]>
Many of those have to be fixed before we can release
<enebo[m]>
We will tag out some things we cannot support (like some of the RubyVM things) and others which are not important enough to hold things up (like small differences in error strings)
<puritylake[m]>
Ya, makes sense, I'll do what I can
<puritylake[m]>
So for the PR for ruby/spec are those the ones in test/mri?
<puritylake[m]>
Or at least the tests that are run by cruby?
<enebo[m]>
We have a mirror of this in spec/ruby in our repo but that all comes from this project
<puritylake[m]>
Ah, I didn't think to match ruby/spec as a repo lol
<enebo[m]>
yeah and in the past this used to even be a project called 'rubyspec' in a different org
<puritylake[m]>
Well Array#[]= isn't testing for errors in ruby spec?
<puritylake[m]>
Sorry my brain is having a slow day lol
<enebo[m]>
puritylake: I don't think aset[]= or slice does in ruby/spec but slice is in neither test suite
<puritylake[m]>
Did some investigating, turns out JRuby's implementation might be right by virtue we are using int's to index, enter a number too long for cruby you get back
<puritylake[m]>
`(irb):1:in `[]=': bignum too big to convert into `long' (RangeError)`
<puritylake[m]>
s/`(irb):1:in `[]=': bignum too big to convert into `long' (RangeError)`/`(irb):1:in \[\]=: bignum too big to convert into `long' (RangeError)`/
<puritylake[m]>
s/`(irb):1:in `[]=': bignum too big to convert into `long' (RangeError)`/`(irb):1:in \[\]=: bignum too big to convert into 'long' (RangeError)\`/
<puritylake[m]>
Code for that is `ruby -e '[0][18446744073709551615..-1] = 1'`
<puritylake[m]>
So it really is system dependent it seems and might be impossible to check in ruby/spec consistently
<puritylake[m]>
Might require a change to `array_test.rb` instead
drbobbeaty has quit [Ping timeout: 252 seconds]
<puritylake[m]>
enebo: at least from what I can see JRuby is implementing it right, it's just CRuby is working with a larger datatype to index
drbobbeaty has joined #jruby
subbu has joined #jruby
<puritylake[m]>
Made the change to `array_test.rb` if this is not the way you want this to be fixed I can undo the changes
<enebo[m]>
puritylake: we cannot change those tests
<enebo[m]>
puritylake: we source them from MRI's codebase
<enebo[m]>
If we cannot fix them then you can add those tests to excludes which will be the file test/mri/excludes/TestArray.rb
<puritylake[m]>
Sorry, will do
<enebo[m]>
No problem. You didn't know. and it sucks that MRI puts so many asserts into a single method
<enebo[m]>
I am a little confused about this though. MRI Range will also go to Bignum after it exceeds its range.
<enebo[m]>
I think they also use a long and they use one bit to mark it (tag so GC knows it is not a GCable object)
<enebo[m]>
Sorry I have to go afk a bit but I will look when I get back. MRI must be doing something here I don't get...
<puritylake[m]>
Also I made a big dumb, I hadn't been passing the excludes
<enebo[m]>
so beg in range I do not think can be a long in this case either even if we are a bit off on max value
<enebo[m]>
well it is ok. I like to run without excludes when the file will run to conclusion just in case I see another related mistake
<enebo[m]>
but running with them is a good idea just to know you did not introduce any new issues
<enebo[m]>
bbiab
<enebo[m]>
puritylake: It is a bit of digging into the C codebase but bignum if it fits in a long (someone constructed a long which would fit into a fixnum) it will get a long value out of it. If it cannot fit into a long it will raise
<enebo[m]>
rb_raise(rb_eRangeError, "bignum too big to convert into `long'");
<enebo[m]>
It goes down to rb_range_component_beg_len to hit NUM2LONG and once you resolve what that is you make it to 2b_big2long (from rb_num2long from rb_num2ulong_inline
<enebo[m]>
./core/src/main/java/org/jruby/RubyNumeric.java: /** rb_num2long and FIX2LONG (numeric.c)
<enebo[m]>
This is another tip. We tend to mark C Ruby methods in our source so you can look for them as comments.
<puritylake[m]>
Ah, thanks for the tip
<enebo[m]>
and fwiw if you look at C and Java we seem to match for num2long
<enebo[m]>
begLen0 should raise if it is a bignum which does not fit
<enebo[m]>
begLen0 on RubyRange that is
<puritylake[m]>
Yup, that was what was throwing the RangeError
<enebo[m]>
ah so we are making a Bignum 1 bit earlier I guess
<puritylake[m]>
Oh no it was the checkInt method was doing that
<enebo[m]>
heheh the numeric stuff always ends up being some long chain of where does X do Y
<puritylake[m]>
Looks that way lol
<puritylake[m]>
Also noticed a fail in test_argf.rb method test_sized_read but it seems it is ruby is making the mistake or that the mri test is wrong but it is what fails the test
<enebo[m]>
It would have been nice if LONGP was just a simple number
<enebo[m]>
system ~/work/jruby master 4831% jruby -e 'p java.lang.Long::MAX_VALUE'
<enebo[m]>
2147483647
<enebo[m]>
9223372036854775807
<enebo[m]>
so we (int)value != value where value is long so yeah I think this should be IndexError like you pretty much had it in the original PR (except you were making 2 exceptions)
<puritylake[m]>
Ok, so my investigating turned out wrong lol thanks for the help
<enebo[m]>
if so then those tests in test_array.rb will pass once you go back to that
<enebo[m]>
yeah no problem. I actually think one of the harder things to reason through is numeric types stepping across boundaries
<enebo[m]>
The other wrinkle in processing parameters to methods in Ruby is sometimes we will decide something is invalid in a different order than MRI
<enebo[m]>
So we end up trying to make sure we check the same thigns in the same order. which is why you see MRI: rb_num2long comments. We pretty much document that it is an equivalent method
<enebo[m]>
In this case MRI is doing these checks inline in that slice method and not calling a helper method
<enebo[m]>
We are calling our own helper checkInt but that is not matching MRI's logic
<enebo[m]>
headius: if you are really reading this (I see your 'h' at the bottom) I found another kwargs error but it is just specificArity JIT method unmarking a ruby2_keywords hash that it shouldn't. So yay since it is not a flaw with the new design
<puritylake[m]>
Is there any formatting program JRuby uses for formatting code? Or is it a "don't treat the files like it was dropped in a wood chipper and we will be fine" sort of thing?
<enebo[m]>
puritylake: we have some style that we encourage and will occasionally reformat code to match it but we do not want a program gate keeping contribution
<enebo[m]>
Usually as people contribute more we will give them more nit-picky formatting suggestions
<enebo[m]>
Here is one if you want to be happier in life...always add { after if EXCEPT if it will be a single line
<enebo[m]>
With most editors/IDEs they will complain about someone indenting wrong but it only takes this happening once to make you make this style rule
<enebo[m]>
As a human scanning code written like that it is surprisingly hard to catch
<enebo[m]>
I only mention this because most younger programmers would not think this could even happen in real life. But as older programmers we have been bitten by this so it maybe only explains why rules like this exists :)
<enebo[m]>
When I started Java we had sad sad tooling :)
<enebo[m]>
and when I started C it was a fucking wasteland
<puritylake[m]>
I always tend to add { after any if, don't think I have used the first one for many years but I have put the em on the same line
<enebo[m]>
puritylake: yeah it is a common idiom now I guess because of history but if you match most people's style in that regard you will get along on new projects easier
<puritylake[m]>
Ya, I tend to try and chameleon my way in other people's code
<enebo[m]>
internally we have a war of 'final'
<enebo[m]>
I personally find marking all things which will not get reassigned as 'final' to be too noisy but I will mark fields and important variables when the importance of it not changing is worth noting
<enebo[m]>
Others like to do it as practice
<enebo[m]>
So you will see somethings heavily marked with final and others not so much
<enebo[m]>
It is not really a big enough issue to demand we all follow a rule
<puritylake[m]>
I would be in the later case, I like to think I am precise in doing so, I guess it make it easier for new comers if they know the value won't change outside the assigned value or constructor, unless it's all caps, then I feel there is no need
<puritylake[m]>
s/later/latter/
<puritylake[m]>
s/later/latter/, s/make/makes/
<enebo[m]>
yeah I think it is more precise but outside fields I wonder how much value it really provides...but it is more a preference I guess
<enebo[m]>
I think what we really need is a clean immutability designator
<puritylake[m]>
Well, I am very confused about my PR, I must have turned back on my format on save and now there is a huge amount of changes (cosmetic) in RubyArray
<enebo[m]>
just whitespace...or is it doing more?
<puritylake[m]>
Moving ifs on new lines of too long and similar changes, was a simple Ctrl+z to get back to original formatting, I'll push that now
<enebo[m]>
ok
<puritylake[m]>
Yup that has been pushed, sorry about that
<enebo[m]>
yeah no problem. I will squash and merge which should I think remove all the original changes
<puritylake[m]>
Perfect
<enebo[m]>
yay jruby-dev-builder is green again
<puritylake[m]>
Nice, that's good to hear
<enebo[m]>
puritylake: merged. thanks for the work on this