<headius>
I just replied about the naming changes, good call, I was just guessing at names
<IlyaBylich[m]>
Well, then it was a very precise guess from your side :)
<headius>
I can get those changes in shortly
<enebo[m]>
Ilya Bylich: Is 7.0.0.9 the last version of Ragel to support -R?
<enebo[m]>
I have 7.0.0.12 and it is missing
<headius>
Yeah looks like Ruby support has been dropped. That doesn't bode well for using ragel going forward
<headius>
Not that I have a suggestion to replace it of course
<IlyaBylich[m]>
Right, it's been dropped. IIRC Ragel is no longer actively developed anymore, but before it's been abandoned author of Ragel announced that Ruby support will be added back
<IlyaBylich[m]>
So 6.10 is the latest version that we can use
<enebo[m]>
ok
<IlyaBylich[m]>
What are you thoughts on using ivars for state management? Is it that slow comparing to lvars?
<IlyaBylich[m]>
Because I haven't noticed any difference on MRI (3.0, without new object shapes)
<headius>
It's likely the overhead is massively overshadowed by other stuff in this lexer
<headius>
At a minimum, even with the best optimizations on MRI, they're still going to be cache validation and the memory access
<IlyaBylich[m]>
Right, at least one level of indirection
<enebo[m]>
It seems too bad this does not have an option to generate a state table ala array
<headius>
It might show up more with YJIT or on JRuby and TR since the register will be much faster than any memory access
<headius>
Without a JIT it's either going to the stack or going to an object in the CPU cache which is why you probably don't see much difference
<IlyaBylich[m]>
enebo: Ragel uses jump tables, at least I see some huge arrays in compiled output
<headius>
If moving some of this stuff to instance variables means we can get this method down to a jitable size that benefit probably will outweigh the overhead
<enebo[m]>
I should probably look at the generated files again. It has been a while
<enebo[m]>
I largely just remember the massive case/when
<IlyaBylich[m]>
Yes, I also think that it's worth it. JITs will be everywhere soon and I'm actually sure that YJIT is able to properly compile it
<IlyaBylich[m]>
enebo[m]: You are right, there's a big case/when, but it's used for actions (i.e. callbacks of transitions), there's a jump-table-based state machine under the hood for transitions.
<enebo[m]>
ah ok
<enebo[m]>
So it uses the table to lookup what actions to trigger and those actions are found in the case
<IlyaBylich[m]>
Racc for example emits every "action" as a method with `_N` suffix that is dynamically dispatched, Bison technically is able to do the same with some magic, but Ragel is the worst of them :D
<IlyaBylich[m]>
enebo[m]: correct
<IlyaBylich[m]>
I'll try to apply approach mentioned in https://github.com/whitequark/parser/pull/899#issuecomment-1359942610 tomorrow to see the impact on MRI. If it's not terrible in interpreter mode I'll check it with JRuby/TruffleRuby/YJIT, if JITed code runs significantly faster I'll probably merge it
<headius>
ok I'm back
<headius>
Ilya Bylich: FWIW even with F0 we are very far off from being able to JIT this, and that node limit on TR is extremely high so I think it will be a while before either of us JIT this code
<headius>
even YJIT might have trouble, I'm not sure, but even if it doesn't it's creating such a massive piece of native code it might not even fit in memory caches
<headius>
I will fix those method names now
<headius>
Ilya Bylich: PR is all set, though I don't have permission to run workflows
subbu has quit [Ping timeout: 260 seconds]
<headius>
ok I'm done with parser stuff for now
<headius>
BuildRangeInstr
<headius>
range isn't even an operand now... we could make a Range operand for pure literal ranges
<enebo[m]>
I believe one problem has to do with it raising on initialize
<headius>
for pure literals we could make that determination at compile time
<enebo[m]>
it cannot raise exception as an operand
<enebo[m]>
yeah that's true
<enebo[m]>
just so we know
<headius>
and I'm not sure what cases could be pure literal but also fail at runtime
<headius>
there may not be any
<enebo[m]>
yeah I just recall there being a bunch of weird specs fixed for 3 but most were things like endless or beginless ranges
<enebo[m]>
they also must be matched literals but I assume you meant that
<headius>
hmm ok we actually do have a Range operand
<enebo[m]>
huzzah
<headius>
it just doesn't cache anything
<headius>
hmm or it's not being used for a simple range like (1..2)?
<headius>
investigating
<enebo[m]>
heh...I am back several hundred commits and I do not see Range as an operand
<headius>
ha
<enebo[m]>
yay
<headius>
org.jruby.compiler.NotCompilableException: no visitor logic for org.jruby.ir.operands.Range in org.jruby.ir.persistence.IRDumper
<enebo[m]>
you added this last a little over a year ago
<enebo[m]>
I wonder what you mixxed
<headius>
how do you see that? my log doesn't show any real changes since 2015
<enebo[m]>
c88ae57
<enebo[m]>
I see stuff in JVMVisitor
<enebo[m]>
yeah I see a Range() in JVMVisitor and a method added to IRBVisitor
<headius>
ok weird intellij is not giving me he right log
<enebo[m]>
HAHA IRBVisitor
<enebo[m]>
I hope I never see that class
<enebo[m]>
anyways this is another example of us having the same conversation
<enebo[m]>
it is kind of funny how much we must enforce each other without realizing it
<headius>
ok I have no idea what history intellij is showing me but I see the commits from last year
<enebo[m]>
err reinforce
<enebo[m]>
yeah it seems to be there using valuecompiler to push range
<enebo[m]>
so that should be caching right?
<headius>
it doesn't seem to be
<headius>
$ jruby -e 'p((1..2).equal?((1..2)))'
<headius>
false
<headius>
oh but it's probably not using the same store for both
<enebo[m]>
oh hahah yeah
<headius>
it could in JIT but probably not in IR
<enebo[m]>
no dedup
<headius>
right
<enebo[m]>
we maybe need some generic deduper
<enebo[m]>
we have enough different types which do this
<enebo[m]>
Almost also makes me wish we just had an annotation on Operand @Deduplicated which magically did it
<enebo[m]>
yeah I just wonder if we can Object these things
<headius>
CRuby
<enebo[m]>
HAHAH
<headius>
so we both do this already but no dedup
<headius>
ok confusion gone
<headius>
whew
<enebo[m]>
Each literal is its own instance and I am not sure we need to dedup ranges
<enebo[m]>
they are small and there are not many of them
<headius>
yeah it's not a big deal
<headius>
caching is sufficient
<enebo[m]>
caching per site definitely makes sense
<enebo[m]>
So I think I just realized why the mocha thing stopped working at the bisect commit I found
<enebo[m]>
it updated Ruby 3 stdlib and gems to default and I looked at those and thought...no way these are causing any issues with backtraces
<enebo[m]>
I just noticed this was the same commit which also bump language version
<headius>
aha
<enebo[m]>
So now I have to see which code path is version guarded in mocha or minitest or rake
<enebo[m]>
The bug is maddening
<enebo[m]>
it takes a block in a test and then adds that block via define_method into a class which extends minitest+mochatest then makes an instance and runs that test
<enebo[m]>
our backtrace shows 'test_me' which is some dummy name they give this method
<enebo[m]>
MRI will show the method name of the place the block was originally defined in
<enebo[m]>
I find this inscrutable since it is actually calling a method call test_me...why would that not be in the backtrace
<enebo[m]>
There will be an explosive aha moment here
<headius>
so is the backtrace issue the bug?
<headius>
or is that just making it hard to find the bug
<enebo[m]>
it is returning wrong method name from processing backtrace/caller
<enebo[m]>
if I print them out raw I can see MRI setting the test_me line as the test_something_else_from_original_file
<enebo[m]>
It is like its original location is somehow overriding its name in the backtrace vs what the method is called
<enebo[m]>
which if I think about it I can convince myself that lexical location of block may be more useful in a backtrace?
<enebo[m]>
but it still needs both since it is also a method that is being called
<enebo[m]>
The more I think about this the more I can see why this is a backtrace like this since the line it fails at is in the original source file
<enebo[m]>
but the source file/line is right. The method name is the define_method one and not the lexically containing one
<enebo[m]>
which sort of feels more right to me
<enebo[m]>
I think one thing we have not implemented is pointing out 'block in method_name'
<enebo[m]>
err I take that back
<enebo[m]>
We do that but we do not do the repeated line thing
<enebo[m]>
block 3 times in or whatever that is
subbu has joined #jruby
subbu has quit [Ping timeout: 255 seconds]
subbu has joined #jruby
sagax has quit [Ping timeout: 260 seconds]
<headius>
got pulled away on some business
<headius>
you might try forcing JIT and see if the JVM trace is more helpful
<headius>
sometimes the interpreted trace gets screwy if people are messing with bindings and stuff but the JVM trace usually just uses the real file and line
<enebo[m]>
I think I figured out the issue
<headius>
nice
<enebo[m]>
Well I definitely figure out an issue and it fixes the reported problem
<headius>
what did I do
<enebo[m]>
procs sent to define_method use their defining hard scope as the name
<enebo[m]>
This has to be a change
<enebo[m]>
you did actually write the code to change this to be the defined method name...in 2008
<headius>
so they rewrite the name before posting the method
<headius>
oh rewriting is actually wrong then?
<headius>
it should use whatever name it had at definition
<enebo[m]>
I need to see if I can observe it changing or not between versions to figure out when it differs
<enebo[m]>
yeah. so test_me which is define_method name is wrong now it should be where the proc came from
<enebo[m]>
This annoys me somewhat
<enebo[m]>
If define_method is an analogue to def then it should report backtraces similarly
<enebo[m]>
The actual file:line is to the proper lexical location where the block is already
<enebo[m]>
I only have one big question on this...why the hell did this work for 9.3?
<enebo[m]>
I never found a version guard
<enebo[m]>
ok my change breaks a bunch of stuff but I changed two values with name
<enebo[m]>
method and callee both use define_method name (which makes sense)
<enebo[m]>
HAHAH
<enebo[m]>
Ruby 2.6 also gives lexical location of proc
<enebo[m]>
So some commit did change this but bisect did not resolve to that commit
<enebo[m]>
and to be fair there was a section of commits which was building jruby.jar but blowing up while erroneously trying to install some default gems
<enebo[m]>
So perhaps I was not testing against newer jruby.jar (but I was fairly sure I was fine by that point)
<enebo[m]>
This won't be too hard to track down now that I realize it is a regression on how we are setting up define_method
<headius>
It definitely makes more sense to use the defined name rather than the lexical surrounding name
<headius>
And of course the lexical file and line number are still there so I don't see why they use the lexical name. As in your example comment, that name might be nothing if the proc came from a script top level
<enebo[m]>
I have a theory
<enebo[m]>
the same proc can be used for n method defs
<enebo[m]>
and they are grabbing location from proc (or something like that)
<enebo[m]>
so they normalized on something which would not change
<enebo[m]>
The other theory is it has been this way so long they don't dare correct it now
<headius>
I guess it's sort of makes sense in a roundabout way but it seems far less useful
<enebo[m]>
well it doesn't really make sense though
<enebo[m]>
who cares what method originally created the proc
<headius>
So now not only do you not know the place where it was defined, you don't even know what name it was defined under
<enebo[m]>
especially since its location is there
<headius>
I don't see how that's better
<enebo[m]>
yeah but mocha leverages the fact that the last caller element will have the test name :)
<headius>
Anyway as you said in the bug, we have to be compatible so of course we'll change it but I think It's wrong
<enebo[m]>
I don't blame them since it seems like it will always work but this is a pretty weird backtrace element