<headius[m]>
a CVE in SnakeYAML has been addressed by updating the library in psych
<headius[m]>
there's one other small change there but it will get released soon
<enebo[m]>
yeah if it affects us we may as well
<enebo[m]>
which I guess the word snakeYAML implies it does :)
<headius[m]>
looks like an entity expansion issue
<headius[m]>
only change required was newer snakeyaml
nirvdrum has quit [Ping timeout: 256 seconds]
<headius[m]>
I am looking into this regression on TCPServer.new reported by logstashers
<headius[m]>
it's minor... of course fixing bind exceptions to be better broke something 🙄
<headius[m]>
I'm going to work with them to add a Logstash CI job
<headius[m]>
they seem to find all our nitpicky bugs, like sequel does
<enebo[m]>
headius: since you looked a lot at these flags....!flag.contains(REQUIRES_DYNSCOPE) == fic.popDynScope?
<headius[m]>
REQUIRES_DYNSCOPE is a static flag determined at parse/compile time I believe
<headius[m]>
pop can change depending on scope optimization
<enebo[m]>
yeah but if it is not required it either is shared or popped?
<headius[m]>
if it is not required it will not be pushed or popped
<headius[m]>
should not
<headius[m]>
it might separately need a binding if it needs to use parent's scope but that's a different check
<enebo[m]>
IRManager has a optimizeIfSimpleScope where it makes sure there is no dynscope requirement
<enebo[m]>
I will leave it as-is but I guess I will need to examine this a little more since it is somewhat foggy on the differences
<enebo[m]>
In any case it is a static flag which should never change
<headius[m]>
yeah
<headius[m]>
hard to tell because we mix compile-time flags and runtime flgs
<enebo[m]>
(except unfortunately some passes seems to reset flags)
<headius[m]>
that's the other wrinkle I ran into
<enebo[m]>
Flags thus far is the only significant issue I see
<headius[m]>
even the static flags get wiped out and recalculated sometimes
<enebo[m]>
the temp vars is not going to be too hard to move around
<enebo[m]>
yeah that is the main issue I see as well
<headius[m]>
calculate flags should not modify in place but instead return a new enum, maybe that would help
<headius[m]>
it's still going to get swapped out and potentially be concurrency issue but anyone can recalculate flags without damaging the ones that are there
<enebo[m]>
I am nearly at a point where fic is everywhere it should be but I have temps and flags as the remaining things I will change after this works (knock on wood)
<headius[m]>
and eliminate FLAGS_COMPUTED
<enebo[m]>
temps will just temp during build and store in startup and then clone and store in full
<headius[m]>
if you have a flags object it should always be computed
<headius[m]>
yeah that sounds good
<enebo[m]>
flags I think can be the same way because at this point the truism stuff I was saying is difficult to reason with
<enebo[m]>
after those two problems the final one (which honestly is more an annoyance) is that we pass IRScope to things like WrappedIRClosure
<enebo[m]>
I do not think we want to reify FIC from IR persistence
<enebo[m]>
but I guess we wouldn't now that I think about it
<enebo[m]>
in any case it smells somehow
<enebo[m]>
90% of this was dead simple though...most of the data for the passes already lives in fic so replacing scope with fic actually either was just a name change or it eliminated some methods on IRScope like the stuff to get/put problem data
<enebo[m]>
but those methods indicated our directional relationship problem because they needed to ask if a fic exists vs just asking the fic for the data
<headius[m]>
really should just eliminate the ic/fic split in IRScope since once we switch over we don't ever go back
<enebo[m]>
They could just replace so long as execution does not reach into scope in any way
<headius[m]>
it's close to that if it's not there already
<enebo[m]>
again fosdem refactoring has made this a lot simpler
<headius[m]>
we had to for dumping
<headius[m]>
yeah
<enebo[m]>
we need more fosdems
<enebo[m]>
well that is totally insane...worked first compile
<enebo[m]>
1592
<enebo[m]>
git diff | wc -l
<headius[m]>
ship it
<enebo[m]>
interestingly only flags and temps I think are the only race now
<enebo[m]>
temps I am not worried about but flags I am but those are next anyways
<enebo[m]>
I just expected to fat finger something
<headius[m]>
ugh this maven gem issue
<headius[m]>
I think I will have to just pull that commit into 9.2 and master now
<headius[m]>
no word from mkristian on helping to fix it so we're stuck for the moment
<enebo[m]>
ok
<headius[m]>
abelsromero: hey are you seeing that "marshal data too short" thing?
<headius[m]>
we need to figure out why that started happening
<headius[m]>
something changed on rg.org
<headius[m]>
enebo: you are working on ir_concurrency branch yeah?
<headius[m]>
I pushed that PR to jruby/jruby so you could use it
<enebo[m]>
no
<enebo[m]>
I made my own and cp'd your two commits
<headius[m]>
all the changes I made are there
<headius[m]>
oh why?
<headius[m]>
it's already in a PR
<headius[m]>
just use that branch and it will be the same PR
<enebo[m]>
well I just didn't
<enebo[m]>
ok so did you do more than those two commits?
<headius[m]>
ok that's fine, but it should be in the PR eventually
<headius[m]>
I have nothing that isn't in the PR
<enebo[m]>
I was not sure at the time I started this whether we would have done some things differently
<headius[m]>
all my other attempts last night were aborted
<enebo[m]>
but once I started I wanted those two commits at least
<enebo[m]>
ok yeah I guess I was not sure what was on it and I just went from scratch
<enebo[m]>
It did not take me long to ask you about that commit though :)
<enebo[m]>
ok well it will end up as PR either way
<headius[m]>
yeah I was unsure why you needed to ask
<headius[m]>
ok
<enebo[m]>
My default is to start from nothing
<headius[m]>
you can push your branch over mine or just move the commits back there but the PR is already created
<headius[m]>
when this goes green I will merge to master
<headius[m]>
will need to rebase ir_concurrency PR to pick it up so we'll have to sync on that then
<enebo[m]>
hah resetState is never called any more
<enebo[m]>
so that is nice
<headius[m]>
yeah I saw that
<headius[m]>
I assume we can delete anything we want from IR since it's not a public API
<headius[m]>
"not a public API" famous last words
<enebo[m]>
yeah well I just changed the signature of about 30 classes
<enebo[m]>
but yeah I consider JIT/IR/AST+parser as non-public
<headius[m]>
damn the torpedoes
<enebo[m]>
a few things in runtime too like StaticScope
<headius[m]>
SS is a bit more of a worry but meh
<enebo[m]>
I mean we already changed a ton of stuff for fosdem with no complaints but internals are internal...even if Java does not have a good mechanism for signalling that
<enebo[m]>
we might not have really change signatures too much there as much as depended on it more
<headius[m]>
mostly adds
<headius[m]>
build changes have been merged everywhere
<headius[m]>
I also just merged to ir_concurrency so it's not rebased
nirvdrum has joined #jruby
<headius[m]>
enebo: merge looks good
<headius[m]>
I'm waiting to hear about psych releasing
<headius[m]>
BindException thing is green and willl merge in a moment
<headius[m]>
if this actually fixes the problem it should also be in .13
<headius[m]>
since we've got a hard segv otherwise
<enebo[m]>
yeah if we can see it prevent one sure
<enebo[m]>
Ok progress is good...temps are no longer in IRScope
<headius[m]>
no reliable reproduction at the moment but several people who seem to be able to trigger it in their apps
<headius[m]>
sweet
<enebo[m]>
So now more or less it IRFlags
<enebo[m]>
It is amazing how much of this was fixing what was in motion but never finished
<enebo[m]>
like most of all the right fields just are already in the right place but the methods still continued calling through IRScope
<headius[m]>
yeah that was what kept tripping me up, I wasn't sure which of those should move to IC or what
<headius[m]>
there were just mutations happening from every direction
<enebo[m]>
flags will be the hardest of the three but I am pretty happy atm
<enebo[m]>
well the goal of moving the things has always been there but it was much harder to see before fosdem due to how much IRScope was passed around
<headius[m]>
yeah for sure
<enebo[m]>
Although mostly it was confusion about how safe it was to change something on IRScope but if you combine that with slowly migrating fields onto IC/FIC then alot of direct scope access just goes away
<enebo[m]>
bleh I guess I do have one error from that...something in BEGIN breaks somehow
<enebo[m]>
HEH a single test in all the tests we run through
<headius[m]>
The goal with the separate frame Fields was to have many different combinations to reduce frame cost as much as possible, but obviously backref and lastline are by far the most interesting
<enebo[m]>
yeah they are fine to have at that granularty
<enebo[m]>
I have always wanted it at that level
<headius[m]>
It is also hard to do them individually without adding individual field sets to the bytecode, which is not ideal
<enebo[m]>
no there is no debate about this from my perspective
<headius[m]>
Or permuting every possible combination
<headius[m]>
Ok
<enebo[m]>
I don't care about them existing or the granularity
<enebo[m]>
I am not seeing many field remove or change their state once set
<enebo[m]>
and for many of these they are set by the time they exit IRBuilder
<enebo[m]>
So most of these flags are essentially read only but I am just trying to understand which ones we can reasonably assume is read-only and safe by the time full starts
<headius[m]>
Yeah at least when I added them they were only ever manipulated during initial flag calculation
<enebo[m]>
I delete initScopeFlags to not remove anything and there seems to be no errors
<enebo[m]>
but manipulated means added only I think
<headius[m]>
Yes
<enebo[m]>
so I think only a single field is toggled back off
<enebo[m]>
BINDING_HAS_ESCAPED
<enebo[m]>
but when this happens might mean this is could be read only by end of build
<enebo[m]>
At some level this is not a permanent solution either
<enebo[m]>
I am just reasoning if we can make sure flags are add only two racing builds will never see something get unset
<headius[m]>
Yeah sure
<enebo[m]>
in the future we should do something else with managing these so we can "as a system" toggle things off
<enebo[m]>
This reasoning is fairly sound to you?
<headius[m]>
Yes
<enebo[m]>
ok then in the morning I will continue to examine the lifecycle of when stuff gets set but I believe the resetting of flags in passes is doing nothing for us but creating this concurrency problem
<headius[m]>
As long as all threads will end up at the same end state it doesn't matter what the intermediate state is
<enebo[m]>
if one thread is ahead of another it will be setting and the other will only just want to toggle it on
<enebo[m]>
it that other thread somehow gets ahead it will just toggle add whatever it hits first
<enebo[m]>
they should see the same state regardless of who is ahead
<headius[m]>
Yeah it seems fine
<enebo[m]>
ok
<enebo[m]>
So hopefully in the morning I will feel better about this being done
<enebo[m]>
I will commit with those removals in initScopeFalgs removed
<enebo[m]>
My only fear with this idea is I am wrong and I end up making things safe but disable some key optimization
<headius[m]>
oh sweet double-free fix confirmed
<headius[m]>
that's a lucky dart throw
<enebo[m]>
heh
<enebo[m]>
it will be nice to have that not showing up
<headius[m]>
yeah gone almost as fast as it showed up luckily
<headius[m]>
I'm still annoyed that Rails switched from sass-ruby to sassc in a minor 6.0.x release
<enebo[m]>
interesting to see in 4 of 5 passes computeScopeFlags is just added as sanity in the same way you would assert
<enebo[m]>
I believe we can just delete those and assume we will not run passes until we have computed flags
<enebo[m]>
The fifth case is super weird though
<enebo[m]>
subbu even added a comment
<subbu>
is that a good thing? :)
<enebo[m]>
subbu: well when I saw WHERE the computeScopeFlags() call was I thought...why is it there!?!?!?? then saw you comment above wondering the same thing
<enebo[m]>
subbu: so yes and no...since it looked strange without your comment but I am happy to see you thought it was odd
<headius[m]>
enebo: I started sprinkling that shite all over the place when I was working on jit
<enebo[m]>
headius: not sure this was from you
<subbu>
i know that once i switch away from code where i am thinking through details, without fixmes, comments, it will be lost .. sometimes those should probably hard asserts instead of FIXMEs .. it is no help commenting "this will likely break in this scenario" and then have that scenario trigger and causing a subtle failure.
<enebo[m]>
ah subbu added it in 2012 :)
<enebo[m]>
/ Why 'Only after running local opts'? Figure out and document.
<headius[m]>
hah
<enebo[m]>
It is funny though...There should be no reason why this would be helpful
<enebo[m]>
I mean by the time local opts pass happens if you call computeScopeFlags on the parent it had to have already conputedScope flags which would mean it is a noop
<enebo[m]>
I guess I will try and prove that
<enebo[m]>
but these computerScopeFlags are mostly harmless but one will unset computed so that one is probably the cause of some of our concurrency mayhem
<subbu>
looks like that original line is from Nov 2009 from 2a3e8a82f2484e055ffdd8b33d0bcebac4d86287 ... so, without digging more, it is possible it is some historical artefact that wasn't relevant in 2012 and I didn't dig into it and lazily added htat ocmment.
<enebo[m]>
yeah I removed it and we shall see if anything happens
<enebo[m]>
I highly doubt it based on looking at this
<enebo[m]>
It probably was just a bit different back then
<headius[m]>
the world was a bit different back then
<subbu>
anyway, i'm not really being helpful here .. but if you need me to poke around anything, ping me and I'll see what I can find.
<enebo[m]>
ACP forcing recompute is probably the bigger mystery
<headius[m]>
yeah I thought that was odd
<enebo[m]>
We more or less only ever seem to delete one flag ever
<headius[m]>
clears COMPUTE and starts it again
<subbu>
ya .. 2009 was when i was still trying to making things work within the constraints of the existing runtime.
<subbu>
many of which probably don't exist now since this is the only runtime.
<enebo[m]>
I guess resetting and removing in initFlags some fields could mean they stop being set
<enebo[m]>
That is really my only fear right now
<enebo[m]>
I do not want to fix our problem by disabling an opt
<headius[m]>
enebo: early next week seems doable for 9.2.13?
<enebo[m]>
headius: assuming we can get someone to verify this will all pay off sure
<headius[m]>
I will get psych released one way or another and then the only remaining items are IR concurrency
<enebo[m]>
it is about 2750 lines of changes so I really want someone to try it
<headius[m]>
we'll have to take a longer look at that thread explosion locking problem
<headius[m]>
yeah we have no repro in hand
<enebo[m]>
fwiw I do not feel most of it is very risky but it is a lot of change
<headius[m]>
but kalenp and johnphillips31416 are around
<enebo[m]>
The flags are the scary part which is part of why I am trying to minimize having to change them for this
<enebo[m]>
good news is we keep reducing mutation in this code
<headius[m]>
thank goodness
<enebo[m]>
once flags is finished there is not much left and what is left is due to inlining which will be broken for a while
<kalenp[m]>
we've got johnphillips31416 banging his head on our CI/build system to get it to run with the nightly and try out that pre-release
<kalenp[m]>
we really want to run this before you cut the release to get ahead of any other issues that might pop up that we haven't seen yet
<headius[m]>
Ok cool
Antiarc has joined #jruby
<headius[m]>
Still in pr but enebo seems to be making good progress
<kalenp[m]>
hitting a lot of internal friction with tooling, but that's how it goes
<headius[m]>
This is the fixing we should have done before but all the previous errors went away with the small fix 🤦♂️
<kalenp[m]>
lol, been there
<kalenp[m]>
seems like this is a hard repro. we haven't been able to hit it with local testing, it's only during our full integration test suite that it comes up
<headius[m]>
Well that makes sense, we fixed the simple case with just a couple threads but you are probably hitting it with many more
<headius[m]>
In any case, this is much more likely to fix "all the issues" but verification would make us all feel better
<kalenp[m]>
I think somebody turned the puma max pool size up to 200 threads, but we only hit that if something is going horribly wrong 😛