<headius[m]>
looking into that perf issue more, I found one item in Symbol#to_proc that helps (need to spend some time optimizing it more) and now seem to have discovered that string interpolation is much slower than in 1.7
<headius[m]>
I reduced his script to pretty much just interpolating a number onto the beginning of a string and we're 20s to 1.7's 13s
<headius[m]>
so looking into that now
ur5us has quit [Ping timeout: 260 seconds]
ur5us has joined #jruby
ur5us has quit [Ping timeout: 260 seconds]
nirvdrum has joined #jruby
valphilnagel_ has joined #jruby
valphilnagel has quit [Ping timeout: 260 seconds]
ur5us has joined #jruby
ur5us has quit [Ping timeout: 260 seconds]
<headius[m]>
enebo: when you are around we should have a chat about this
<headius[m]>
The way IR structures dstr makes it difficult for me to improve the logic on the JIT side. I'd like to be able to just evaluate all elements and then piece them together as a batch, but it's compiled as a series of AsString operations that store into local vars, and then the locals are used to in the dstr
<headius[m]>
but that means every object has to pass through RubyString on the way to being appended, which is a waste for many core types we know how to convert to ByteList directly
<headius[m]>
a naive dstr benchmark with a lot of loop overhead is around 50% slower than 1.7, so the cost is substantial
<headius[m]>
FWIW on Graal escape analysis those boxes do go away, now that I've fixed up how AsString is compiled
<headius[m]>
but it's still wasteful
<enebo[m]>
Do you mean DStr or EvStr?
<headius[m]>
DStr
<headius[m]>
look at the IR for something like "foo#{blah}bar"
<enebo[m]>
Just trying to see where AsString fits in as I only see it used in EvStr
<headius[m]>
all the operands to BuildCompoundStringInstr are previously either literal strings or from AsString
<headius[m]>
or look at the building and you'll see it
<headius[m]>
it unconditionally evaluates and does AsString on every interpolated element, where at least in 1.7 we used to specialize a couple of those (like numerics) to skip the intermediate RubyString object
<headius[m]>
but the operands are just local vars by the time I get to compiling BuildCompoundString
<enebo[m]>
ah this is quite weird
<enebo[m]>
I do not see where asString happens in source :)
<enebo[m]>
It does not change what I planned on saying though
<enebo[m]>
we just make asByteList for types we know we can make as bytelists
<enebo[m]>
like we do with FrozenString
<enebo[m]>
It is unfortunate without a lot of pass work we cannot determine whether something is initialized as a single def:use and then we could just continue to use asString but mark it as something which can extract the byetlist
<enebo[m]>
doing it with another instr is simpler
<enebo[m]>
err I guess it must just be EvStr itself
<enebo[m]>
ah yeah so AsString comes from EvStrNode
<enebo[m]>
we just examine EvStr body and if it is a simple literal we can just replace AsString with AsByteList and then eliminate indirecting to RubyString
<enebo[m]>
The idea of using a pass to determine this and specializing how we emit AsString has a little appeal but it feels like a lot of work when we just know it during build time
<headius[m]>
well that's not exactly the problem
<enebo[m]>
it is one problem though right?
<headius[m]>
we do need to actually call to_s when appropriate
<headius[m]>
and use the String logic in that case
<enebo[m]>
yeah so some AsString is still there
<headius[m]>
the optimization in 1.7 basically just bypassed calling to_s when it's a Fixnum or Float or whatever
<headius[m]>
hmm
<enebo[m]>
I can actually do this a bit better
<headius[m]>
maybe the instr we really want here is "AppendString"
<enebo[m]>
this could be an operand
<enebo[m]>
and more or less same as FrozenString but ByteList or something like that
<enebo[m]>
IRBuilder can see FixnumNode and then extract the bytelist at this point
<enebo[m]>
then temp var for pieces are either: AsString(), FrozenString(), or ByteList()
<enebo[m]>
in fact we can probably even eliminate FrozenString() in this case if it has any overhead over a new ByteList()
<headius[m]>
I have one problem with this
<headius[m]>
we may not even want the bytelist
<headius[m]>
I believe what we need is a way to take an unmolested value and tack it onto a dstr-in-progress
<headius[m]>
BuildCompoundString right now basically jits like this: evaluate and asString every element, create a blank string for the base, append each literal part and asStringified part in turn
<headius[m]>
so the options I see are either moving the brains of AsString into BuildCompoundString so it can work with values directly, or we reassemble how dstr works
<headius[m]>
i.e. either BuildCompoundString implies AsString-or-optimized-append of each interpolated element, or we have something like EmptyString, evaluate element, AppendAsString, evaluate element AppendAsString
<enebo[m]>
Another thing to mention is a second loop over pieces to pre-compile static parts into a single part
<enebo[m]>
during irbuild
<headius[m]>
yeah dunno if that's a problem right now or not
<headius[m]>
but it would be good to fix if it is a problem
<headius[m]>
it's a conundrum for me because another idea I have would be to just use invokedynamic and push, push, push, push all the elements and then let indy sort it out like Java string interpolation does now
<enebo[m]>
so moving AsString into BuildCompoundString is not really a problem to me at all
<headius[m]>
that would let all my ideas work
<enebo[m]>
It seems to literally only exist for building compound strings
<headius[m]>
it just means a pretty fat instruction conceptually (not in code, it's pretty simple)
<enebo[m]>
well yes and no
<headius[m]>
I think some of these coarse operations got broken up a little too fine in hopes we'd be able to see through more in IR opto
<enebo[m]>
It is more logic but what value to do we get out of AsString as an instruction? I do not think it would ever be used by a compiler pass for an optimization
<headius[m]>
in this case it just ends up making things worse because we don't know that operand A belongs with dstr B
<headius[m]>
hmm yeah
<enebo[m]>
In that sense we actually have made things more complicated by breaking this out
<headius[m]>
FWIW I checked CRuby iseq and they don't have an AsString per se
<headius[m]>
interpolation compiles as a dyncall to to_s (in the iseqs directly) followed by "tostring" which is the "anyToString" logic on the bottom of AsString
<enebo[m]>
yeah this was likely an early IR design thing where at least I did not realize only a single instr consumes its result
<headius[m]>
basically it does dyncall to to_s and if it's not a string then it tries another way to force it
<enebo[m]>
It may be that it was looked at as a counterbalance to knowing something was a cacheable literal to frozenStringLiteral
<enebo[m]>
but in the case of this instr IR does not really even use that operand in any passes
<enebo[m]>
So I believe live value as pieces does make sense
<enebo[m]>
It also removes AsString from existing
<headius[m]>
bleh I broke a weird tainting case with my AsString optimization work
<headius[m]>
it's supposed to call to_s and then taint the resulting string if the original object was tainted
<headius[m]>
🙄
<enebo[m]>
yeah
<headius[m]>
only failure on my small_optz branch so far
<enebo[m]>
so we good on this...just remove AsString which literally just means evStr will build it as a normal value
<enebo[m]>
the buildcompoundstring can decide what values need to call to_s or not
<headius[m]>
I'm fine with it
<headius[m]>
so anywhere that expected an AsString to happen now does it themselves
<enebo[m]>
ok. I can see how this gives us more knowledge at execution time and it does not hurt IR being able to eliminate anything
<enebo[m]>
'anywhere' == DXXX I guess more or less
<enebo[m]>
Although DXstr probably is also a buildcompoundstring
<enebo[m]>
so I think it is all isolated to that instr
<headius[m]>
yeah it just does the same thing and then execs it
<enebo[m]>
My idea of merging would not really be very likely without value propagation happening
<headius[m]>
bleh I don't want to fix this tainting
<headius[m]>
it's deprecated in 2.7 and gone in 3
<headius[m]>
maybe we can start just dropping it
<enebo[m]>
good question...I think ultimately I wonder how much of this will get noticed by tests
<enebo[m]>
you also run mri tests?
<headius[m]>
yes, that's where it failed
<enebo[m]>
Part of me does not believe we are too solid on taint checks currently but I guess I don't know for sure
<enebo[m]>
ok
<headius[m]>
everything else is green
<headius[m]>
we're way off on taint checking to the point that I am not sure we do it right at all anymore
<headius[m]>
we may be a little better on propagating but it's meaningless anyway
<enebo[m]>
yeah it is my "feeling" but heh I do not know of any specific examples
<enebo[m]>
I have always thought tainting was a poor model
<enebo[m]>
well you can leave it off with a FIXME perhaps
<headius[m]>
pondering for a bit longer if I can fix it easily
<enebo[m]>
and then when no one reports and we make it to 3 we delete the FIXME
<enebo[m]>
ok
<headius[m]>
it's just a bunch of work because the way I modified this it just compiles AsString as a call, so it goes into to_s and optimizes right
<headius[m]>
to fix this I have to add a bunch of bytecode around it to re-taint, or rig up the dyncall to also taint somehow
<enebo[m]>
Part of this feels academic...Does ANYONE use tainting any more? Did they ever?
<headius[m]>
I don't know if they ever did
<enebo[m]>
I am all for being compatible but knowing something is going away and we do not probably support it that well as it is makes me feel this is not a big priority
<headius[m]>
people used safe levels but mostly for the coarser-grained "disable eval and exec" kind of things
<headius[m]>
taint was never reliable
<enebo[m]>
I hated it in Perl and more or less people would just use the by-pass pattern which would mean reexamining those (which were not simple to see) and decide if someone actually considered tainting or just hacked past it
<enebo[m]>
in probably 90% of the cases they just hacked past it
<enebo[m]>
which completely destroyed the safety
<enebo[m]>
I have no doubt someone has successfully implemented a program with tainting and was safer from it...in the same way I feel someone has written a million line C program without any memory access exceptions
<headius[m]>
hah yeah
<headius[m]>
it's just such a swiss-cheesy way to do security
<headius[m]>
you have to be checking this flag everywhere anything might be unsafe, which means you're sure to miss something
<enebo[m]>
In one sense this feels like the difference between mutexes and using some paradigm/model like actors
<headius[m]>
yeah I suppose it's similar
<enebo[m]>
tainting is more like mutexes where every interaction needs to be scrutinized to death whereas a class of problems disapprar if you just pick some abstract which solves some/many problems for you
<headius[m]>
protect every place you might deal with shared mutable state, or only pass around immutable state
<enebo[m]>
anyways I think tainting is a poor system for security (but I am no security expert :) )
<headius[m]>
hmmm
<headius[m]>
oh right I understand what you were saying now
<headius[m]>
I forgot EvStrNode is basically AsString
<headius[m]>
what types of nodes can end up in a DStr?
<headius[m]>
I'm hoping it's all either strings or EvStr
<enebo[m]>
I may have a comment in parser/lexer...I was tracking through these
<headius[m]>
I will special case EvStr in the build of compound strings for not
<headius[m]>
for now
<headius[m]>
if it's evstr, just evaluate the value directly knowing it will go into BuildCompoundString and be stringified
<enebo[m]>
yeah
<enebo[m]>
I believe compound strings are StrNode and EvStrNode
<enebo[m]>
Part of me wonders if there is a degenerative case of a DStr also directly containing a DStr as one of its elements
<headius[m]>
hmm starting to play with this change and I kinda wish we could have instrs in instrs
<headius[m]>
AsString is an instr because it does a call
<headius[m]>
look at dynamicPiece in IRBuilder
<headius[m]>
it's used by four Dxxxx compiles
<enebo[m]>
sure but they are are for BuildDynamicString right?
<enebo[m]>
err compound string
<enebo[m]>
shoot not regexp
<headius[m]>
BuildDynRegExpInstr
<enebo[m]>
I wonder why not :)
<headius[m]>
DSym and DStr do the same logic
<headius[m]>
both are BuildCompoundString
<headius[m]>
hmm
<headius[m]>
DRegexp is the only one that has its own instr
<enebo[m]>
ah ok I see
<headius[m]>
the other three all backend on BuildCompoundString
<enebo[m]>
it is preprocess
<headius[m]>
str, sym, xstr
<headius[m]>
ah right
<headius[m]>
incrementally processing the elements in terms of regexp
<headius[m]>
is there anywhere else other than Dxxx that EvStr gets used?
<enebo[m]>
I think the D* are the onyl things which can
<enebo[m]>
So 4 of them 3 of which use buildcompoundstring
<enebo[m]>
dregexp just needs to_s() logic put into it
<headius[m]>
ok I'm fine with that
<enebo[m]>
I am looking at parser just to convince myself
<enebo[m]>
although I was sure before I started looking
<enebo[m]>
word lists can receive them but then make a DStr when it sees them and puts it within
<headius[m]>
heh BuildCompoundString doesn't need any changes
<headius[m]>
well a small change to call this short circuited append logic
<headius[m]>
I want to make EvStr a hard error in IRBuilder
<enebo[m]>
you mean if not found in a D*Node?
<enebo[m]>
or do we not call generically through build?
<headius[m]>
yeah
<headius[m]>
I want it to only be handled by dstr and not accidentally fall into general build
<enebo[m]>
yeah makes sense
<headius[m]>
I suppose if it doesn't now it won't then but if it does not we need to know why
<headius[m]>
does not = does now
<enebo[m]>
yeah I mean you should know shortly
enebo has joined #jruby
<headius[m]>
ooh I got another optimization
enebo has left #jruby [#jruby]
<headius[m]>
anything we know can convert into a string directly, like a fixnum, can just write into the string we're building directly
<headius[m]>
zero alloc unless it has to grow
<enebo[m]>
yeah I guess I was suggesting that above
<enebo[m]>
or I should say I thought that was what you were trying to do as part of this
<headius[m]>
goes to show you how much fruit remains in our compiled optimization
<enebo[m]>
it was why I said the first stuff I said
<enebo[m]>
if we could do value propagation we could probaly also pregenerate more of the string
<headius[m]>
well I mean the original plan above was to just get a ByteList from a Fixnum and use that
<headius[m]>
but there's no reason to even have the bytelist if I already have a target in hand
<headius[m]>
just write into it
<enebo[m]>
yeah
<headius[m]>
it's not even visible across threads for dstr so it's like super safe
<enebo[m]>
fixnum still needs to be in some representation to be added to the string
<enebo[m]>
it is one case where we know it will be 7bit ascii though
<headius[m]>
btw I want to revisit 32-bit fixnums too
<headius[m]>
it's just a storage/allocation tweak... all logic would keep working with the value as a 64-bit long but we'd allocate 4 bytes fewer for 32-bit range
<headius[m]>
probably not much value below 32 bit
<headius[m]>
unless we want to add a Fixnum that just stuffs its 8-bit value into the header
<enebo[m]>
allocation in JIT you mean
<headius[m]>
might be able to fit 16 even
<headius[m]>
allocation in JVM I mean
<enebo[m]>
I guess I don't know what you mean now
<enebo[m]>
have 3 different RubyFixnum classes?
<headius[m]>
new RubyFixnum types that have an int field instead of a long field
<headius[m]>
yes
<enebo[m]>
ok
<headius[m]>
they all are just Integer for Ruby now anyway and we already have the smarts to use multiple backing objects for Integer
<enebo[m]>
yeah I guess so long as methods largely are still mono
<enebo[m]>
most of these would just return getLongValue() or whatnot
<headius[m]>
most would be... somewhere we need to access the right field though
<headius[m]>
right
<enebo[m]>
we need unions in java
<headius[m]>
that is where the tradeoff would need to be analyzed
<enebo[m]>
how many fixnums do we ever even allocate?
<headius[m]>
another way would be to have a Fixnum32 and extend that in Fixnum64 that just adds another 32-bit field and we mask and shift every time :-)
<headius[m]>
or use unsafe to access as a single field
<headius[m]>
😎
<enebo[m]>
unsafe solves all problems
<enebo[m]>
you know for BuildStringLiteral we could provide a start hint on size
<enebo[m]>
if we have n StrNodes we know how large they are
<enebo[m]>
the dynamic parts will make it larger but it will be a better starting point
<headius[m]>
bytelist API sucks who wrote this
xardion has joined #jruby
<headius[m]>
hmmm
<headius[m]>
int to string goes from lowest digit... how gross is it for me to just reverse them in place as a second pass
<headius[m]>
I guess I can calculate how many digits and do it from the end
<headius[m]>
got it
<headius[m]>
well this ought to eliminate any fixnum interpolation perf issues forever