lucasb has quit [Quit: Connection closed for inactivity]
ur5us has quit [Ping timeout: 246 seconds]
ur5us has joined #jruby
nirvdrum has joined #jruby
nirvdrum has quit [Ping timeout: 256 seconds]
drbobbeaty has quit [*.net *.split]
BlaneDabneyGitte has quit [*.net *.split]
XavierNoriaGitte has quit [*.net *.split]
slackfan[m] has quit [*.net *.split]
ipproxy[m] has quit [*.net *.split]
FlorianDoubletGi has quit [*.net *.split]
fzakaria[m] has quit [*.net *.split]
TimGitter[m] has quit [*.net *.split]
HarlemSquirrel has quit [*.net *.split]
lopex[m] has quit [*.net *.split]
ipproxy[m] has joined #jruby
HarlemSquirrel has joined #jruby
fzakaria[m] has joined #jruby
drbobbeaty has joined #jruby
BlaneDabneyGitte has joined #jruby
TimGitter[m] has joined #jruby
slackfan[m] has joined #jruby
lopex[m] has joined #jruby
FlorianDoubletGi has joined #jruby
XavierNoriaGitte has joined #jruby
ur5us has quit [Ping timeout: 272 seconds]
nirvdrum has joined #jruby
shellac has joined #jruby
shellac has quit [Quit: Computer has gone to sleep.]
shellac has joined #jruby
shellac has quit [Quit: Computer has gone to sleep.]
shellac has joined #jruby
shellac has quit [Quit: Computer has gone to sleep.]
shellac has joined #jruby
shellac has quit [Quit: Computer has gone to sleep.]
shellac has joined #jruby
shellac has quit [Client Quit]
shellac has joined #jruby
shellac has quit [Ping timeout: 265 seconds]
xardion has quit [Remote host closed the connection]
xardion has joined #jruby
<headius[m]> enebo: so I'm trying to reimpl coverage without event hook
<enebo[m]> headius: cool
<headius[m]> so far I just introduced a CoverageInstr that updates coverage data directly... no line hooks necessary
<headius[m]> runs fine without --debug
<headius[m]> oneshot will be the real magic though since the JIT can turn it into a switchpoint
<enebo[m]> I am curious on instr overhead in interp
<enebo[m]> I am hoping it is not too big but at times I have noticed it to be linear
<enebo[m]> Also is it always next to a LineInstr?
<headius[m]> yeah I have considered merging that
<headius[m]> CoverageLineInstr that does both
<enebo[m]> If you do then JIT can just emit those as two things but then interp will just be doing thosei n Java vs incrementing ipc and looking up etc...
<enebo[m]> headius: the proposed goal is for it to always be ready to turn on without --debug right?
<headius[m]> in JIT the line number instructions are just metadata; they don't introduce any bytecode
<headius[m]> yeah that's right
<enebo[m]> I feel like you can just leverage lineinstr and make it a little more complicated instead of adding an additional instr (mostly because it ends up rippling through serialization and stuff)
<headius[m]> so if you use coverage it will just be on for line instrs
<enebo[m]> I guess it really comes down to whether the costs of this are small enough but doing it to LineInstr feels the most optimistic
<enebo[m]> At one point I ran out of space for representing an instr as 1 byte (although we have removed a number where we currently are not in jeopardy of that)...so that is part of my motivation :)
<headius[m]> right, since we already have the line overhead anyway
<enebo[m]> but it will help interp perf to keep it in line
<headius[m]> in interp at least
<enebo[m]> I assume all the code would be the same in JIT
<enebo[m]> anyways my 2c
<headius[m]> this may be able to save some overhead by caching the int[] of coverage counts as well, but the real goal is making oneshot
<enebo[m]> I am yakking around a little bit today
<enebo[m]> I got sick of thinking on how to really eliminate all idutil calls against strings for a bit and started removing deprecations
<enebo[m]> right now I want to trade tokline being an ISourcePosition with just being an int
<enebo[m]> In the grammar as tokens it will end up boxing as Integer but all other places it will just be used as int
<enebo[m]> But ISourcePosition is not really needed
<headius[m]> yeah nice
<enebo[m]> Just noticing some things while cleaning up deprecations
<enebo[m]> there are 3 left which you may want to look at
lanceball has quit [Ping timeout: 256 seconds]
Caerus has quit [Ping timeout: 256 seconds]
lanceball has joined #jruby
<headius[m]> ah one thing this will do is add filename to LineNumberInstr
<headius[m]> so that's an extra reference for every one
Caerus has joined #jruby
<headius[m]> coverage needs the filename to lookup the array of lines
<enebo[m]> headius: Scope has file can't we use that since we record scope
<headius[m]> how?
<headius[m]> ah well I guess LineNumberInstr doesn't interpret itself
<headius[m]> so this is special logic in interpreter to provide file for coverage
<enebo[m]> so you are wondering how to do it in interpreter or JIT?
<enebo[m]> we have it in JIT so this is just abotu interp?
<headius[m]> this will also add the lineInstr.isCoverage check always on
<enebo[m]> InterpreterContext.getFile() should work for interp
<headius[m]> in interp
<headius[m]> yeah I can get filename in interp one way or another
<headius[m]> that's fine
<enebo[m]> ok
<headius[m]> no concern about making lines always check coverage?
<enebo[m]> some concern that it will somehow be a big hit but I don't think we can know until we try it
<enebo[m]> If we are going to do it less instrs will mean less overhead in interp
<enebo[m]> for JIT I am assuming it will be close to zero cost off unless it somehow slows warmup a tiny bit
<enebo[m]> I actually have this feeling off it will be a single object compare and that overhead will not be seen in interp
<enebo[m]> on it will have overhead obviously but that is fine. It will be less than the old trace impl
<enebo[m]> Since again it will be within Line in Java vs another instr
<headius[m]> a single instruction will definitely be cheaper than two
<enebo[m]> yeah in some small methods you can see linear overhead based on number of instrs vs what those instrs do
<enebo[m]> so CISC is not a bad concept for our interp :)
<enebo[m]> so long as bigger instrs do not inhibit analysis
<enebo[m]> ah early lunch...I hear feet moving :) bbiab
<headius[m]> ok basic line is hooked up... going to try a little indy magic
<headius[m]> woot
<headius[m]> oneshot appears basically free as expected
<headius[m]> need to wire up the API to enable it but this is basically working
<headius[m]> looks like I almost have it hooked up
<headius[m]> I had to propagate coverageMode through IRScope constructors though... it was just using flags before but I need more than one bit
<headius[m]> not sure if that's the best way to go or not
<enebo[m]> headius: you mean for live state of whether coverage is happening?
<headius[m]> well there's multiple coverage modes now
<enebo[m]> but each scope will need to know which mode it is in?
<enebo[m]> Isn't this a global toggle and/or thread toggle?
<headius[m]> well, I need to get it somewhere and it was using flags before
<headius[m]> so they each knew via flags that coverage was enabled, but I need multiple modes
<headius[m]> if it's possible to not have scopes know about coverage at all and only have it in IRBuilder that would be good
<enebo[m]> I am missing something about how this works...at parse time we know what coverage mode
<headius[m]> ParserSupport installs coverage mode into RootNode and that propagates through IRBuilder and scope creation
<headius[m]> oh I see
<headius[m]> it's because of lazy method
<enebo[m]> is it a lexical feature...maybe I just need to read about this instead of ask questions
<headius[m]> so IRClosure and IRMethod keep it on hand
<headius[m]> it is parse time you are right
<headius[m]> but it's only set into root node so IRMethod needs to memoize it
<enebo[m]> heh..what is this called again? I need to read something
<headius[m]> it memoized it before by looking at parent.getFlags(
<headius[m]> look for uses of IRFlags.CODE_COVERAGE
<headius[m]> and IRBuilder.needsCodeCoverage
<headius[m]> those need to be int mask of coverage modes
<headius[m]> I am naively just propagating the int instead
<headius[m]> I can push branch shortly so you can see the change
<enebo[m]> so JIT will examine that once when it compiles and emit something but in interp we need to access those flags
<enebo[m]> headius: look at InterpreterContext
<enebo[m]> there are cached computed fields which came from IRScope.flags but I move them there because it is for execution and it avoids having to ask flags every time
<enebo[m]> At this point it does not need to do that at all but I am just pointing out for things the interpreter looks at all the time they get accessed via IC instead of IRScope
<enebo[m]> we do have access to IRScope though but after all that work before fosdem I like the idea of IRScope being for analysis and IC for execution (at least for the interpreter)
<headius[m]> what am I supposed to see
<enebo[m]> you can see a field like private boolean pushNewDynScope;
<headius[m]> this is only used at build time, or in the case of IRMethod, at lazy build time
<enebo[m]> and how it is setup and accessed
<enebo[m]> oh ok so nothing to be checked at execution
<headius[m]> right
<enebo[m]> then ignore all that
<headius[m]> so it's just this int mode that I need to have available when lazy compiling method
<headius[m]> since it can't go in flags it is a new field
<enebo[m]> what is this feature called again?
<headius[m]> Coverage.start(oneshot_lines: true)
<enebo[m]> ok
<headius[m]> kind of an awful name
<enebo[m]> I don't get how this is lexical
<enebo[m]> It just gets toggled in with a start and then you can require a million files and see them all in result
<headius[m]> well it is but at the coarses possible grain... it's enabled for a whole file
<headius[m]> what do you mean lexical
<enebo[m]> well every single file after start will need to have this coverage won't it?
<headius[m]> yes
<enebo[m]> It is not something you add to each specific file you want coverage for
<headius[m]> not really no... it is enabled globally and alters how all files during coverage are compiled
<headius[m]> but we lazily compile some things
<enebo[m]> Feels like some state which would be on Ruby and asked from that. I think I am confused because you said it is on root node
<headius[m]> yes but I don't have that state when we go to compile an IRMethod lazily
<headius[m]> all I have is the DefNode and whatever state was put into IRMethod at build time
<headius[m]> and its parents
<headius[m]> and that would not be appropriate to check lazily anyway since it could have been turned off after load but before we lazily compile
<headius[m]> so it needs to be memoized somewhere IRMethod can see it
<enebo[m]> so if I get this all the source before starting coverage should not be doing coverage so all files after that point need to be marked but older ones will not. Since methods are lazy we cannot assume a method being built is before or after?
<enebo[m]> I wrote that out while you were talso typing
<headius[m]> right
<headius[m]> though I suppose if we turn coverage off we stop checking for it
<enebo[m]> This feature sort of makes sense and sort of doesn't
<headius[m]> well there's no real reason it needs to be in IRScope except that I need to be able to get it from IRMethod when lazily compiling
<enebo[m]> yeah so it is mark these files may be coverage files and then something to make sure it should actually still be doing it
<headius[m]> and it used flags before, so this is just expanding that to an int field
<headius[m]> it was already being stored in there but not enough bits for me
<enebo[m]> I will point out that IRMethod is made immediately at build time but its contents are not
<enebo[m]> so you could proabbly do this globally instead of passing it in as part of node and all children inherit parents status (since methods would be marked before being processed)
<enebo[m]> Sorry I am asking a lot of questions but I am wondering if DATA or whatever old school coverage is is the under the covers the same feature or not
<enebo[m]> I do not mean exact same feature but I mean can both be running at the same time
<enebo[m]> heh OMG I don't even remember how that feature works :)
<enebo[m]> ok so I see what is up there
<enebo[m]> That is some ancient code
<headius[m]> yeah I'm just not sure where to propagate through
<enebo[m]> so I am curious on 2.6 if you can end up using generic coverage which makes the global hash and coverlines and have it use a different once
<enebo[m]> At this point no one would be using both so it is probably not a big deal but I am curious how weird this feature is
<enebo[m]> Like can I start/stop and start/stop and on second result do I still see the original start stop covered files?
<headius[m]> I don't think so... it clears result when you stop
<enebo[m]> but that would nuke the generic feature although perhaps that is how MRI does it
<enebo[m]> who would do both :)
<enebo[m]> I realize reviewing this original code that we could set this up much simpler by recording last newline (or LineInstr if started making this much later than during AST) and then construct a single zeroed out array
<headius[m]> you can't do lines and oneshot_lines together
<enebo[m]> I guess though the problem may be not all lines are actually tracked
<enebo[m]> This growth algo is horribly bad
<enebo[m]> it basically does n+1 arraycopy
<headius[m]> it should just allocate however many lines are in a given file at parse time
<enebo[m]> well newline nodes could just be counted or whatnot too
<headius[m]> the impl of oneshot is a little weird in CRuby... it seems to allocate zero-length and then push line numbers onto it
<enebo[m]> yeah this was likely modelled after MRI
<enebo[m]> part of me thinks it would add 1 to non-soiurce lines ot something
<headius[m]> so that's working with this rig
<headius[m]> note it only triggered line 2 once
<headius[m]> I can push this and we can discuss how to remove the coverageMode state from IRScope
<enebo[m]> well it only cares about whether it was visited right? Not how many times?
<enebo[m]> hmm
<headius[m]> yeah
<headius[m]> I'm just turning it off after the first one
<enebo[m]> we could easily do this as a bit structure as well
<headius[m]> it could be a BitSet or something
<enebo[m]> anyways...the moral of the story is get it working first :)
<enebo[m]> If we can just do # of lines make a bunch of 0 bits we can make this free during parse/build more or less as well
<enebo[m]> right now it is pretty expensive on parse
<enebo[m]> AH
<headius[m]> I don't know why it does the lines differently for oneshot
<enebo[m]> So I think it only marks reachable lines as 0
<enebo[m]> everything else is nil and will never be reached
<enebo[m]> so a coverage tool only cares about the ints
<enebo[m]> which means we need 3 states
<enebo[m]> Their impl is interesting that they use Fixnum
<enebo[m]> I suppose they have no real cost for that
<headius[m]> so that's them turning it off and pushing the line number on that array
<headius[m]> but I don't get why
<enebo[m]> ah ok they did what we were talking about above
<headius[m]> I guess the output is just different?
<headius[m]> ah yeah
<headius[m]> `{"/Users/headius/projects/jruby/blah.rb"=>{:oneshot_lines=>[1, 2, 3, 5]}}`
<enebo[m]> HAHAH
<headius[m]> so oneshot outputs an array of lines that were encountered
<headius[m]> rather than an array of length = number of lines with a count in each
<enebo[m]> I am confused again
<enebo[m]> ok
<enebo[m]> but why oneshot_lines => ?
<enebo[m]> Can other stuff be in this at the same time? future proofing?
<headius[m]> well I think you can do oneshot_lines along with method or branch coverage but we don't implement those yet
<headius[m]> you can't do oneshot lines and non-oneshot lines at the same time
<headius[m]> so yeah the main issue with accessing this coverage mode is that it comes in from parser on RootNode
<headius[m]> so it's state that's carried along via parser/compiler path and not via access to runtime
<headius[m]> I'll give you a minute to look at that while I get more coffee
<headius[m]> so there's always-on coverage showing zero overhead for oneshot_lines
<headius[m]> no warnings about tracing and no --debug
<enebo[m]> A number of things confuse me but none of them are your PR yet
<headius[m]> FWIW we could also do tracing always on this way (without binding support) but it adds an indy per line
<headius[m]> after code size/complexity, binding is really the problem for always-on tracing
<enebo[m]> I need to remember what the thing is about all nodes needing to know they are needsCoverage
<enebo[m]> in AST which then is now translating into LineInstr
<headius[m]> yeah I don't know if it's for anything except propagating to compile phase
<headius[m]> we have the coverage mode at parse time so we stick it in RootNode, and then IRBuilder gets it from RootNode
<headius[m]> but it doesn't need to live in either
<headius[m]> and after compile it doesn't need to live anywhere
<enebo[m]> but why would I have added that as a boolean everywhere for a global coverage tool
<headius[m]> 🤷‍♂️
<headius[m]> I'm following your lead on this PR
<enebo[m]> I mean at this level then I see no reason for lineinstr to also record this
<headius[m]> well we do want to compile line instr differently when coverage is on
<enebo[m]> not having to stick it in there would reenable caching lineinstrs
<headius[m]> yeah but do we want every line to have to go look at runtime to see if coverage data is there?
<enebo[m]> well we need to know a unit of code is doing coverage over all or for a particular line?
<headius[m]> it's per file, but we want to make this access pretty cheap
<enebo[m]> My guess is some line number marked nodes are coverage nodes and some maybe are not?
<headius[m]> if we're going to share an instr
<headius[m]> well, I'm just doing it where we did it before PR
<headius[m]> if we're not sharing an inster then we are back to CoverageLine instr and no flag check
<enebo[m]> yeah I am just trying to understand this
<enebo[m]> I feel like it is a per scope check and that would mean all lineinstr would not need extra state
<headius[m]> if we are sharing an instr then we have to have a way to know if a given line should update coverage data
<headius[m]> sure, it can be, but we don't want to have to walk all parent scopes every time we encounter a LineNumber
<enebo[m]> but if it is per scope it is a boolean check either way
<enebo[m]> but it assumes all lineinstr in a scope are either on or off
<enebo[m]> I mean each IRScope would need to mark their parent so we are not walking and ultimately that start would make it into IC itself
<enebo[m]> mark their parent == mark same state as parent
<headius[m]> whether the boolean is on scope or line makes little difference to me
<headius[m]> I'd prefer not to check it at all and compile a different line instr under coverage mode
<enebo[m]> well I am just saying this will add megabytes of IR instr data this way unless we add more caches for it
<headius[m]> well, only when running with coverage on
<enebo[m]> yeah I am talking about that
<headius[m]> otherwise it still pools LineNumber instrs
<enebo[m]> I believe it can pool either way...but you are right we can eliminate a boolean check in interp by making another instr
<headius[m]> we are also talking almost exclusively about interpreter too
<enebo[m]> but since we decided to share it I figure a single boolean per scope is better than one per lineinstr
<headius[m]> JIT does it the smart way with indy
<enebo[m]> but then I circle back to why needsCoverage is the way it is in the AST
<headius[m]> JIT don't care if it's a new instr or not
<enebo[m]> sure this has always been about two things for me: a) interp not being impacted too much b) understanding why this is per node/instr vs just some global state on the AST/IRScope
<headius[m]> or hell, via context.setLine
<enebo[m]> This second one I think I just need to look at this a little bit
<headius[m]> though that seems a bit deep to put coverage logic
<enebo[m]> If interp used two instrs it would just make the dispatch path one longer but I am not sure that is better or worse than a single boolean check on a field
<headius[m]> the old logic added Trace instrs under coverage
<enebo[m]> but your PR is doing that boolean field per instance of line so that is why I am just looking backwards here wondering why the AST is doing any of this
<headius[m]> so in terms of size it's basically the same overhead
<enebo[m]> oh so more than just lines
<headius[m]> well old logic did the line number instrs (pooled) plus a trace per line in coverage mode
<headius[m]> so 2x instrs for line in coverage mode
<enebo[m]> well I was only bringing up the extra booleans on lineinstr because the cache is not working when it is on but it would be if it could one global boolean per scoper
<headius[m]> and I think it might also have added a second trace instr for LINE event because it didn't distinguish
<enebo[m]> yeah I think you are right
<headius[m]> one for LINE and one for COVERAGE along with original LineNumberInstr
<enebo[m]> I think it just did two because we said fuck it tracing is already expensive
<headius[m]> I had to split this from Trace because it' snot a trace anymore, so then it's a matter of reusing LineNumber or adding a new one
<enebo[m]> yeah so I am on board reusing obviously
<headius[m]> then it's a matter of making non-coverage LineNumber have close to zero cost compared to coverage
<headius[m]> boolean local to the instr is obviously most direct way
<enebo[m]> My confusion is just why we have it per instr vs per IC/IRScope as a check since the later if it works would make lineinstr cache continue to work
<headius[m]> I need a boolean on it to turn off coverage for oneshot_lines anyway
<enebo[m]> oh I see
<enebo[m]> you do not want to keep marking if it was marked
<headius[m]> oneshot_lines needs stateful LineNumber
<enebo[m]> and you do not want to get to see you do not have to set
<headius[m]> regular line coverage does not
<headius[m]> this is also implementing the "truly oneshot" design I mentioned in that issue... if IR instr gets encountered for oneshot, it *will not JIT with coverage*
<headius[m]> so for most lines this won't even make it into jit if you specifiy oneshot
<headius[m]> if it does make it into jit it will update coverage once and then no-op
<enebo[m]> ok read a bit more
<enebo[m]> Is there a case where LineInstr will pass in coverage = false in the constructor? Or did you do that to disambiguate the constructor
<headius[m]> disambiguate I think
<enebo[m]> I don't think so now that I looked added a comment to the PR
<enebo[m]> Perhaps it just looked more reasomable to say 'yes I want coverage but that coverage is either oneshot or not'
<enebo[m]> I would consider LineInstr.newCoverage(linenum, oneshot check)
<enebo[m]> or some static helper so it ends up not being what is that true for
<headius[m]> sure
<enebo[m]> one reason I am fixating on that is line.coverage is actually two things at the same time
<enebo[m]> one to indicate we want coverage but secondly to indicate if we finished covering that line
<enebo[m]> I could linguistically say that in a way where they mean the same thing but that is how I viewed that variabled as I was reading
<enebo[m]> so I like the notion of not seeing that variable be very prominent
<enebo[m]> I don't know why I go so deep on explaining stuff like this :) It does not really matter very much
<headius[m]> it is indeed two things
<headius[m]> I could add another field :-D
<enebo[m]> please don't :)
<enebo[m]> maybe a comment in what it represents
<headius[m]> we also need to add support for "methods" and "branches" coverage but I think they'll be completely separate tracking
<enebo[m]> and it was not rocket science to figure out either obviously but in 3 years when we talk about this again I can just read it is dual purpose
<headius[m]> enebo: pushed an update
<headius[m]> I eliminated the boolean args to LineNumber constructor and just pass mode through
<headius[m]> also fixed oneshot result output to be a list of encountered lines, as in MRI... I think the functionality is basically done now
<headius[m]> if you have an idea about reducing how many places we propagate this mode I'm all ears
<enebo[m]> ship it
<headius[m]> enebo: ok so might need your help somewhere in here
<enebo[m]> lol
<headius[m]> that 1 should be a 2, 9 should be a 10, and 21 should be 20
<headius[m]> I have to fix the order... now I know why they push the lines as they are encountered
<enebo[m]> It need not be order specific does it?
<headius[m]> they want this order to match first encountered order
<headius[m]> it does
<enebo[m]> ah I see
<headius[m]> so that's weird but whatever
<headius[m]> the off by ones I am not sure how to tackle
<enebo[m]> pretty weird requirement for something which stops keeping track after it is visited once
<enebo[m]> you just need to give small cases on what is off and we can figure it out
<headius[m]> that's the test so you can see the lines they expect
<headius[m]> things like which line shows up in a multi-line hash literal
<headius[m]> I don't quite get these lines
<headius[m]> like why 2 but not 1 or 2
<headius[m]> 1 or 3
ur5us has joined #jruby
<enebo[m]> why the fuck is 2 valid?
<headius[m]> 🤷‍♂️
<headius[m]> you see my problem
<enebo[m]> seriously makes no sense to me at all
<headius[m]> I will fix ordering but not sure how to adjust these other lines
<enebo[m]> so both 2 and 20 are from not being the line of the start of the [ or { but the first elment within it
<enebo[m]> or so it seems
<enebo[m]> that really is weird
<headius[m]> right
<headius[m]> but the multi-line calls use the first line
<headius[m]> so like wat
<enebo[m]> that probably is not too hard to fix as I am sure in first case I used front literal and in second one it ends up as end of structure
<enebo[m]> OTOH this is done via newline node
<enebo[m]> and that is a field on node
<enebo[m]> it is possible by fixing it I will break something else with positioning
<enebo[m]> So a) wrong position on node which has newline set b) newline is on wrong actual node
<enebo[m]> headius: how could 25 and 20 be out of order?
<headius[m]> 20 isn't encountered until after the call on 25
<enebo[m]> yeah I know that
<enebo[m]> I am asking why it is broken for us
<enebo[m]> it is when you append the result?
<enebo[m]> line is after the call?
<headius[m]> oh because I just used coverage data to generate the list
<headius[m]> walking lines from beginnign to end and looking for nonzero
<headius[m]> MRI actually pushes the lines onto a list as encountered for oneshot mode
<headius[m]> so I'm doing that now but I need an IntList and we don't have one 🙄
<enebo[m]> yay
<headius[m]> hah we do but it's hidden in another class
<enebo[m]> I wonder if mri AST will specify newline so I can meaningfully compare
<headius[m]> MethodData.IntList
<headius[m]> enebo: oh I just saw the terrible growth algorithm in ParserConfiguration
<headius[m]> I'm changing that to just update line count and then construct it all at once
<headius[m]> why does it create this array as it goes anyway?
<enebo[m]> who knows?
<headius[m]> hmm
<headius[m]> Zero out coverable lines as they're encountered
<enebo[m]> I think I know
<headius[m]> so this is marking lines that can actually be covered with 0
<headius[m]> so like end of a multi-line block stays -1
<enebo[m]> there are two states initially
<enebo[m]> nil and 0
<enebo[m]> so nils represent unimportant lines which are not visitied
ur5us has quit [Quit: Leaving]
<enebo[m]> but the reason why this grows is for inputstreams piped to Ruby
<enebo[m]> It is still done horribly innefficiently but there may be no end
<headius[m]> well yeah but if not for the 0 or nil it could just create it once we have all lines
<enebo[m]> or I guess it has to end
<enebo[m]> but we cannot look at the file
<headius[m]> we don't start executing anything until parse finishes
<enebo[m]> so the count newlines would work but we also need nils
<enebo[m]> I think now that we IRBuild we could do this then
<enebo[m]> we can alloc perhaps at end of parse keeping tally but in irbuild make nil or 0
<enebo[m]> or however we signify the not interesting lines
<headius[m]> yeah unclear... this info may not be available by the time we are in IRBuilder?
<enebo[m]> well it is if we init to not interested to right size based on newline nodes marked
<enebo[m]> then each new lineinstr we mark those offsets as 0
ur5us has joined #jruby
<headius[m]> oops
<headius[m]> ok there we go
<headius[m]> so close
<headius[m]> beyond this I'm not sure what to fix
<headius[m]> I updated last commit with properly working oneshot_lines output
<headius[m]> so it's there and functional but off by one in a few places
<enebo[m]> yeah I am looking at the simple assignment off by one but this is no a ton of fun :)
<headius[m]> this is likely affecting output for non-oneshot mode fwiw
<headius[m]> hah ok
<headius[m]> I'm going to have some late lunch, brb
<enebo[m]> I think stmts (makes newline) of stmt of command_asgn of lhs ''=' ...
<enebo[m]> so lhs.line is the newline
<enebo[m]> err the whole thing not the lhs
<enebo[m]> that does a node_assign
<headius[m]> That line 2 output is the most confusing to me
<enebo[m]> fun times...yeah I change this and the world may break or it may fix many things
<enebo[m]> I think I interpret this as first data element of array or hash ends up being the line
<enebo[m]> why? I don't know but two cases are an array and hash in this example and they both seem to do it
<headius[m]> I am glad to see this oneshot mode has no overhead, but I knew indy can do that
<headius[m]> Yeah, that seems to be the explanation, such that it is
<enebo[m]> 20 and 2 are those eamples
ur5us has quit [Ping timeout: 252 seconds]
<enebo[m]> Struct.new { looks like the same thing but it is adding newline I guess because it is a call with a block
<enebo[m]> "such that it is"
<headius[m]> So it is sort of like whatever the first line that actually adds elements to the array?
<enebo[m]> I am drawing off a very small data sample :)
<headius[m]> There are probably other tests in here with similar failures
<enebo[m]> well this will be messed up...if we do not do exactly the same thing when building up these listish structures in our parser then our newline may wrap the whole thing or just be different
<headius[m]> I don't know that we have ever spent much time getting this green
<enebo[m]> I would like to know all of them if I am to bother fixing this...It also means making sure tracing and ripper and what not do not regress from it
<enebo[m]> the problem there is that nodes and newlines are only the same thing in both impls if they are attached to the exact same thing
<enebo[m]> newline itself being attached is an impl detail but being attached to the wrong thing is I guess a semantic issue
<enebo[m]> Had these been nodes like they used to then it might actualyl be easier to change without fearing other breakage
<enebo[m]> As a general aside how the hell do coverage libraries cope with this info anyways?
<enebo[m]> I mean 2 for something which spans 1-4? How does that work
<headius[m]> Yeah I was wondering that too
<headius[m]> Like why is it not a problem that line 1 and line 3 don't show up
<headius[m]> I guess because they would have to have executed? But that's not really true either if the line 2 hash creation fails for some reason
<enebo[m]> It is possible we do not break those apps
<enebo[m]> even with being off by one
<enebo[m]> I keep thinking there is no way they can know to add these without some other info like source scanning heuristic or maybe using ripper data or something like that
<enebo[m]> ripper or similar seems like only sane option
<enebo[m]> 2 happens to be on an :array which spans 1-4 boom 1-4 covered
<enebo[m]> I guess it all boils to how simplecov works
<headius[m]> yeah one thing I'm not clear about is how they know lines that don't need to be covered
<enebo[m]> but I do not get how this gem can specify this new feature and simplecov picks it up
<headius[m]> I guess they don't in this output?
<headius[m]> with the original output you get nils for those lines
<headius[m]> but oneshot output is only a list of covered lines
<headius[m]> PR is green as is but of course I didn't untag anything
<enebo[m]> so I just saw that on twitter...that simplecov does not use this output
<headius[m]> these lines being off probably doesn't affect normal line coverage that much because the other lines are nils
<headius[m]> coverage is usually looking for uncovered lines
<enebo[m]> I think it may transform to other format and then use simplecov
<headius[m]> yeah
<headius[m]> not yet they don't
<enebo[m]> I believe simplecov uses parser so it will probably find largest expr which is on that line and mark the whole thing
<enebo[m]> it is a bummer it does not use ripper although I remember whitequark mentioning trying to do ripper version
<enebo[m]> not sure that was more than a I should try it
<headius[m]> enebo: I think I will merge this as is and move on to other things
<enebo[m]> yeah it is definitely a step up and if you run the ordinary slow version ti will be the same
<enebo[m]> so it is not like we are doing any worse
<enebo[m]> I will look into this although I was just told it is Friday
<headius[m]> and this works without --debug now
<enebo[m]> I shit you not
<headius[m]> haha
<headius[m]> yeah I have been having trouble remembering what day it is
<enebo[m]> I seriously thought today was Thursday
<headius[m]> I haven't been out of the apartment in a week, and only to get groceries in the past month
<enebo[m]> For anyone reading this...we both work at home and so we are not confused by that as much as I guess never leaving the house
<enebo[m]> Going out to eat or shop during the week and it is easier to remember which day it is but now...it is getting fuzzier
<enebo[m]> Last three weeks I have only went out to the garage 2-3 times and brought trash down and back up to garage
<headius[m]> hah yeah
<enebo[m]> maybe outside like 9-10 times in the last month and only my yard
<enebo[m]> living the dream
<headius[m]> PR is merged
<enebo[m]> may delivery continue to be viable
<headius[m]> I will update that --debug issue guy and let him know where we stand
<enebo[m]> collio
<headius[m]> lines are not right but it's working
<enebo[m]> yeah I am curious if this works as-is with different lines
<enebo[m]> It would be nice to be matching but those hash and array line numbers are just weird to me
<headius[m]> yah I don't know how his tool uses this output
<headius[m]> I updated the issue though
ur5us has joined #jruby
ur5us has quit [Ping timeout: 252 seconds]
<headius[m]> I keep internally debating whether the cost of extra invokedynamics is worth always-on tracing and it just doesn't seem like it
<headius[m]> especially now that coverage works without debug mode