<headius[m]>
you were saying we could try to get this and jffi into .18 but I think you meant .17
<enebo[m]>
hah...yeah sorry I seem to be off-by-one
<enebo[m]>
kares: we should land it now
<kares[m]>
enebo: go for it
<enebo[m]>
ok
<headius[m]>
enebo: ah I was reviewing
<headius[m]>
there was one concern
<headius[m]>
--no-rdoc and --no-ri will be errors now
<enebo[m]>
Sorry I caused a lot of confusion. I thought he was going to merge this yesterday so I am surprised to see you review today but I missed that detail
<headius[m]>
anyone getting latest RG other ways will have dealt with this already
<headius[m]>
if anyone is using jruby as a complete jar or something and using those flags they will error now
<headius[m]>
it is a small concern
<headius[m]>
we were like a year behind though
<enebo[m]>
we could patch if we are really concerned
<headius[m]>
review is in with the one comment about this
<headius[m]>
we could revert the one diff there potentially and keep the flags for embedded users
<enebo[m]>
Part of me feels this is something everyone has to deal with anyways so it is not really tied to 9.2 from a support perspective
<enebo[m]>
headius: We are not specifying the --no-foo right? You are concerned about embedders having added those?
<headius[m]>
I agree, it is just a (small) feature removed in a minor
<headius[m]>
yes that is my concern
<headius[m]>
embedded users also tend to lag behind upgrading things in general
<enebo[m]>
I mean I agree it is something removed so we could patch someone elses software we ship to make it stay the same. I have mixed feelings
<enebo[m]>
JRuby did not remove these options but they cannot just downgrade simply either
<headius[m]>
right
<enebo[m]>
Well it is landed so that one commit if it is just one could be reverted and we don't need to talk about this ever again since we likely will never release RG on 9.2 again
<headius[m]>
yeah
<headius[m]>
I can put together a diff or PR for this small reversion
<enebo[m]>
I see no downside to letting those flags not error on 9.2
<enebo[m]>
sure. or even just commit it
<headius[m]>
oh heck you know what I may be wrong anyway
<headius[m]>
I read the diff wrong... it removes the flags but they also errored before
<headius[m]>
what was removed is just the recommendation to use --no-document before quitting
<headius[m]>
also this includes the install of RG during build right?
<headius[m]>
so it is not versioned in our repo?
<kares[m]>
yes
<kares[m]>
we remove all versioned rubygems files from lib/ruby/stdlib
<kares[m]>
and install rubygems on lib module bootstrap
<headius[m]>
that's cool
<headius[m]>
getting more and more of Other People's Code out of our repo
<kares[m]>
oh I see there's conflicts
<kares[m]>
let me see if I can rebase and resolve
<headius[m]>
ok... master is no rush so if you need to look at this tomorrow it is fine
<kares[m]>
ok, conflicts resolved but I rather let CI run through the PR again ...
<headius[m]>
ok
<headius[m]>
I can push the button once green if you need to go
<enebo[m]>
kares: thanks. was having a late lunch
ur5us_ has joined #jruby
<chrisseaton[m]>
Is there a recommended Java version for JRuby?
<headius[m]>
not really
<headius[m]>
8+
<chrisseaton[m]>
Like if I report a performance bug on 8 are you going to ask me to retry on 16?
<headius[m]>
yes but that would be normal investigation
<headius[m]>
is this related to your dig thing?
<chrisseaton[m]>
Yeah do you want that? It's slower with indy was my point - didn't mention that in the Tweet. It is faster than MRI so it's not slow in general.
<chrisseaton[m]>
Slower with indy on 8 at least.
<headius[m]>
sure
<headius[m]>
I don't know why indy would affect it but it's worth checking
<chrisseaton[m]>
Is your recursive? Probably related to that?
<headius[m]>
well, it is basically the same code as CRuby and has not had much changes in 6 years
<headius[m]>
iterative might be faster but there aren't really any indy calls in the logic
<enebo[m]>
It is basically entirely internal and Java so I don't even know why indy would matter
<headius[m]>
we can look at it though
<chrisseaton[m]>
Ah well this surprised me - MRI is actually iterative! But it's not clear from the code!
<headius[m]>
we may not have incorporated some improvement they made
<chrisseaton[m]>
The semantics are to recurse with each element, so you do have to have some calls or equivalent guards to match the semantics.
<headius[m]>
it hasn't come up as an issue so we have not investigated
<enebo[m]>
we verify it is builtin and just call Java directly so there is some cost but we will dyn dispatch if it is no longer builtin
<headius[m]>
yeah this logic changed
<chrisseaton[m]>
Yes we similarly just added a 'verify it's a builtin logic'
<enebo[m]>
chrisseaton: you have that benchmark public?
<headius[m]>
when kares deprecated isBuiltin he basically also broke it... later I undeprecated it but did not notice the breakage
<headius[m]>
you can see the isBuiltin above that expects builtinCache to be populated but it never is
<headius[m]>
so it just fails and looks it up over and over again
<headius[m]>
I will come up with a minimal fix for .17 in PR
<enebo[m]>
yeah I see. we never even set builtinCache
<headius[m]>
(this does not change the indy perf diff but does improve it overall)
<enebo[m]>
so in this case it would largely only be some other knock on bug where cache.typeOk ended up having a bug
<enebo[m]>
if not then this just allows us to exit early and not churn a new cache entry
<headius[m]>
yeah the worst case would be if somehow the cache was broken but that would be a much larger pervasive issue
<enebo[m]>
I highly doubt typeOk is broken
<enebo[m]>
yeah we would see other problems
<headius[m]>
FYI I have that jffi.extract.name thing working properly and added two additional test runs that use the packaged jar in either temp loc or specific name
<headius[m]>
will have dude reverify
<enebo[m]>
yass
<headius[m]>
enebo: kares I have merged RG on master
<headius[m]>
and 9.2 branch is merged
<headius[m]>
another 9.3 PR out of the way
ur5us_ has quit [Remote host closed the connection]
ur5us_ has joined #jruby
<headius[m]>
these dig numbers are weirdly noisy
<enebo[m]>
yeah
<enebo[m]>
I applied and tried the cache fix and it is mostly faster but seemingly bumps up and down a bit per arity test
<enebo[m]>
gmm although I see on 9.2 we are using a deprecated version of isBuiltin
<headius[m]>
oh yeah I should undeprecate there too
<headius[m]>
I did so on master some time ago
<enebo[m]>
I think I patched the wrong version :)
<headius[m]>
it is a convenient method and I don't think it needs to be deprecated
<enebo[m]>
err no I didn't
<headius[m]>
actually I just realized dig calls the other one that wasn't broken
<enebo[m]>
yeah
<headius[m]>
so this is broken in the non-deprecated one
<enebo[m]>
but it does not perform a setCache
<headius[m]>
but fixed by PR anyway
<headius[m]>
enebo: see my patch in PR, I just avoid using builtinCache altogether
<enebo[m]>
oh intere4stnig it just does a direct assign
<headius[m]>
I am not sure why it was introduced
<headius[m]>
so this will not improve dig but will improve the numeric fast-path call sites that use this isBuiltin
<headius[m]>
seems like those are the only consumers
<headius[m]>
enebo: might have seen this lookup in profile just due to other numeric sites
<headius[m]>
benchmark-ips alone is doing a lot of number stuff
<enebo[m]>
perhaps so but if so then we should see the entire bench get quicker
<enebo[m]>
If we can even trust sampling
<headius[m]>
it seems a little quicker but the noise makes it hard to tell
<enebo[m]>
An iterative dig would spend less time digging through the args array beyond anythign the JVM may get confused by with a lot of recursive calls
<headius[m]>
wow G1 affects this bench a ton
<headius[m]>
43M IPS versus 64M
<headius[m]>
damn G1 is the worst setback in perf we have had in years
<enebo[m]>
parallel
<headius[m]>
I am doing a comparison run with parallel now
<headius[m]>
the benchmark doing one call to dig per iteration is also an issue for us because the block activation is nontrivial
<enebo[m]>
I feel later jvms + par seem to win all the time especially with indy on
<headius[m]>
the dig call is super fast so the noise is likely the block overhead
drbobbeaty has quit [Read error: Connection reset by peer]
drbobbeaty has joined #jruby
<headius[m]>
heh, the dig calls seem to be a tiny part of this bench
<headius[m]>
at least for small numbers of args
<headius[m]>
I removed dig and the ips barely changed
<chrisseaton[m]>
BIPS did change - now evals I think?
<enebo[m]>
If I make iterative remove crap and any dyncall potential dig-10 only goes from 10M to about 14M at 1-3 it is identical. I can confirm at the low end dig is virtually none of this measured time
<enebo[m]>
measure { 1 + 1} is 33M vs dig-01 which is 24M so I can see a change in perf but 1 + 1 is pretty minimal work
<headius[m]>
That will be enough for the jvm to eliminate it completely
<enebo[m]>
doing empty block
<enebo[m]>
{} is 39M
<headius[m]>
BIPS is a nice tool but still has too much overhead in the harness for tiny measurements
<enebo[m]>
I think though as a microbench this is being overshadowed by time invoking the block
<headius[m]>
CRuby will have similar issues though blocks are a little cheaper there
<headius[m]>
This explains to me why the CRuby and JRuby curves were almost identical, most of the work is running the bench harness
<enebo[m]>
yeah MRI tops at 31M i/s for {}
<enebo[m]>
that is 2.6 but I don't think we will see a large difference
<chrisseaton[m]>
Does that mean the indy problem is with block yields instead?
<enebo[m]>
I tried 3.0 is slower than 2.6 at 28M i/s which I think is expected
<enebo[m]>
dinner but if we want to address dig with iterative approach we should measure the cost of dig
<headius[m]>
It may or may not be faster. It will inline better I guess
drbobbeaty has quit [Read error: Connection reset by peer]