[HParker]

Now we can emit line events and line numbers like Ruby does!

example,

1 +
  2 +
  3

[1,
  :RUBY_EVENT_LINE,
  [:putobject, 1],
  2,
  [:putobject, 2],
  1,
  [:send, {:mid=>:+, :flag=>16, :orig_argc=>1}, nil],
  3,
  [:putobject, 3],
  2,
  [:send, {:mid=>:+, :flag=>16, :orig_argc=>1}, nil],
  [:leave]]]

Related: #667

[jemmaissroff]

Thanks for working on this! Left a couple qs

[jemmaissroff]

Can we make this an if !(...) instead of the empty if?

[jemmaissroff]

What does this mean?

[HParker]

I should have deleted this. I initially just malloced this to size 100, but forgot to remove the comment. I think the realloc way isn't that hacky, though I wish I didn't have to store it this way.

[jemmaissroff]

I would expect C to complain here because I think strlen returns a size_t which is unsigned, not an int. Is there no warning?

[HParker]

Ah! it does warn! I wonder if we can turn those warnings into CI failures.

[jemmaissroff]

Good call, I'll look into it!

[enebo]

When I see this I am unsure if this will match how MRI does this today. In the current grammar it marks nodes as newline. Those nodes are the ones that emit line numbers. Those occur in stmts and top_stmts in the grammar (parse.y). The method you are using maybe works the same but I think you will emit some line numbers differently (although I guess you can prove that by comparing iseqs made by both). It maybe is doing more work here since grammar marks in a single place and then you just check if you are in a place which needs a line number vs checking all nodes (or insns) to see if line number changed.

I also wanted to comment here because serialize also has to be capable of emitting line numbers. I half wondered if parser should mark newline nodes like MRI/JRuby does or whether you should be comparing every node/insn to see if jumped to a new line?

[jemmaissroff]

@enebo thanks for the thoughts!

but I think you will emit some line numbers differently

Can you say more here? Which line numbers do you think will be different?

whether you should be comparing every node/insn to see if jumped to a new line?

And then marking the node?

[enebo]

@enebo thanks for the thoughts!

but I think you will emit some line numbers differently

Can you say more here? Which line numbers do you think will be different?

I think maybe it is possible for something containing a statement but is not a statement to emit the line number before where it would have naturally been emitted. Or not :) I just see it as a different mechanism for the same thing but it is unclear to me it will behave exactly the same. Figuring out this is complicated if I look at module (which contains stmts) I see it is 'primary' if I burrow down the grammar I see stmts -> stmt -> expr -> then I get lost so I cannot tell if module would be a stmt or not. If not then you would be putting a line before it would be with the current parser.

This would be where comparing iseqs could probably show it one way or another. If compatibility is paramount to merging with MRI then I think you will need to do this anyways (unless compile is comparing iseqs already).

whether you should be comparing every node/insn to see if jumped to a new line?

And then marking the node?

I think if the line number check is only when a node is marked it would be less work but only because I think many nodes are not marked newline. It is trading looking for any node which changes the start line to just ones with something on a struct. From my perspective I see serialize and compile having to replicate the same logic even if this does work. So it may be more convenient during parse than during compile+serialize. With that said adding data to a struct has its own tradeoffs.

The other issue (and this perhaps should be in a discussion is having to binary search to determine line will have some impact over potentially storing the line while the parsing is originally occurring. I don't want to complicate things though.

[enebo]

As it turns out I forgot I mark newline nodes in JRuby's AST tool. ModuleNode is marked newline so it must be a stmt. But I guess Ruby's grammar is so complicated I have no confidence this PRs technique will work exactly the same.

[enebo]

Having thought about this a bit I think my reservation about it not working the same is probably not my main one. It is having to do this in multiple places (seralize/compile) vs it happening in parse. Even if it gets put into parse my concern about it working the same would probably still be there since statements in YARP may not line 100% with stmts/top_stmts but:

1. Logic would only happen in one place
2. It potentially could avoid the binary search to line number thing if it was integrated into the parsing part.

So I don't think my comments should stop this landing but I do think some more thought should get put into where line number calcs happen.

[jemmaissroff]

Thanks for the thoughts @enebo. We can look into the serialization case. What serialization API for newlines would make most sense to you? We're wary of attaching newline information to every node struct because this would bloat the nodes unnecessarily. I wonder if attaching the same newline_locations list would make sense instead?

Then we could pull that list calculation out of compile, and re-use it for serialization, so the logic wouldn't be duplicated

[eregon]

So on the example from the description CRuby only emits 1 line event for all 3 lines?

I don't know well about this area, it would be useful for me to understand what we need those line markers for: #667

[enebo]

@jemmaissroff ultimately what we serialize should have as little post-processing as possible when we deserialize since deserialization is an extra step to be able to use this native parsing. For JVM, YARP is about not paying the warmup cost (and obviously the benefits of a shared parser). So serialize.c probably should add lines for newline nodes into the AST so we are not doing the same amount of work that compile.c is doing in Java (vs in C).

It sounds like, at this point, the strategy will be to beef up what we need during serialize. If it become obvious this is a lot of extra processing then I will open something up at a later date. Right now I am only trying to get enough running to see how well it currently works.

[HParker]

I am still trying to understand what the tree would look like with newline nodes attached. is the idea,

"any node that changes the line number should include a newline child node"

a dump => load of the example in the description gives,

ProgramNode(0...13)(                                  
  ScopeNode(0...0)([]),                               
  StatementsNode(0...13)(                             
    [CallNode(0...13)(                                
       CallNode(0...7)(IntegerNode(0...1)(), nil, PLUS(2...3)("+"), nil, ArgumentsNode(6...7)([IntegerNode(6...7)()]), nil, nil, "+"),
       nil,                                           
       PLUS(8...9)("+"),                              
       nil,                                           
       ArgumentsNode(12...13)([IntegerNode(12...13)()]),
       nil,
       nil,
       "+"
     )]
  )
)

Would a tree including line numbers look like,

ProgramNode(0...13)(                                  
  ScopeNode(0...0)([]),                               
  StatementsNode(0...13)(                             
    [CallNode(0...13)(                                
       CallNode(0...7)(IntegerNode(0...1)(<LINE 1 NODE>), nil, PLUS(2...3)("+"), nil, ArgumentsNode(6...7)([<LINE 2 NODE>, IntegerNode(6...7)()]), nil, nil, "+"),
       nil,                                           
       PLUS(8...9)("+"),                              
       nil,                                           
       ArgumentsNode(12...13)([<LINE 3 NODE>, IntegerNode(12...13)()]),
       nil,
       nil,
       "+"
     )]
  )
)

I don't think this is right.

I am still struggling to understand where it would make sense to include line number nodes if we are only emitting them when the line number changes since execution can go forwards and backwards across line numbers.

[HParker]

Alternatively,

would runtimes have all the line information they need if location nodes include a line number?

[enebo]

@HParker Sorry I used the wrong terminology. There was a point a long time ago that newlines were actual nodes. At some point we all switched to marking nodes with a boolean field (or flag in MRI's case: NODE_FL_NEWLINE) which just indicated we care about the line of that node. What is a little stranger about the concept is some nodes marked as newline may have the same line as another which is why you see that empty if in iseq_compile_each0. This is very similar to your approach but you lack the flag so you try this on more stuff.

In the future if we realize we need flags (e.g. RNODE.flags in MRI) then it would reduce processing time in compile.c because it would be doing less binary searches. Until that point I guess it makes more sense to make overall progress.

[enebo]

Alternatively,

would runtimes have all the line information they need if location nodes include a line number?

This is how MRI and JRuby work today but they store what is rb_location_t (JRuby only adds end_pos for the few nodes which need them vs all of them -- like tracing end line for module). With that said, YARP does generate all the needed SyntaxErrors beforehand so many line numbers may not be needed by the time you are at compile.c. If you do still need them for error reporting specifically start and end offset can derive them. Error reporting is uncommon and it will be a single binary search to get a line. So not a big deal. I will say YARP is saving quite a bit of memory by only recording 2 ints vs 4. Tradeoffs.

For line numbers in compile and line for trace events I had recently talked to Kevin and he was open to the notion of making those more readily at hand. We did not talk if that was just serialize or in the parser so it was just a desirable thing. From just our last two comments I guess there are two aspects:

1. which nodes care about emitting a line (NODE_FL_NEWLINE)
2. how do you get that line (MRI: rb_location_t, YARP: binary search)

[HParker]

Happy to split it up, but since the conversation is happening here, I added support for line:col information on location nodes in serialize. I think this is a potential way to do newline information in serialize without adding anything to the node.

ProgramNode(0...13)1:0-3:3(
  ScopeNode(0...0)1:0-1:0([]),
  StatementsNode(0...13)1:0-3:3(
    [CallNode(0...13)1:0-3:3(
       CallNode(0...7)1:0-2:4(IntegerNode(0...1)1:0-1:1(), nil, PLUS(2...3)1:2-2:0("+"), nil, ArgumentsNode(6...7)2:3-2:4([IntegerNode(6...7)2:3-2:4()]), nil, nil, "+"),
       nil,
       PLUS(8...9)2:5-3:0("+"),
       nil,
       ArgumentsNode(12...13)3:3-3:3([IntegerNode(12...13)3:3-3:3()]),
       nil,
       nil,
       "+"
     )]
  )
)

[eregon]

What is a little stranger about the concept is some nodes marked as newline may have the same line as another which is why you see that empty if in iseq_compile_each0.

Are those "duplicates" needed? I remember specifically for coverage we need to ignore these extra "fake" newline-marked nodes (I can try to find the TruffleRuby issue about this where we discussed it with @enebo), so it seems even better if these nodes are not "wrongly" marked as newline in the first place.

[eregon]

Found it: truffleruby/truffleruby#2125 (comment)

It's not coverage but seems the same case of don't want two newline-marked nodes per line.

[eregon]

I will say YARP is saving quite a bit of memory by only recording 2 ints vs 4.

Maybe it's fine to limit max line and column to 2^16 (64k), in which case it's the same as two ints. But still likely more memory than varints for offsets.
It would also mean the need to have an array of line number to byte offset to be able to extract the relevant substring in the source. So that's one extra indirection for reading from the source. OTOH line&col->offset is O(1) with that table vs O(log n) for the binary search for offset->line&col.

[eregon]

I think overall we need to better understand the requirements here before jumping to implementation, how about scheduling a call next week (not Monday for me)?

[enebo]

I think if there is a memory efficient way to store line(s) for newline marked nodes back at the parser we should have that conversation but I also don't think we should hang up compile.c progress. In my current spike I have probably about 40 FIXMEs about things not being sent (or that should change). We will all be making changes as we learn from making progress. Getting further into these activities will uncover them faster. I am game to talk about this topic whenever though.

@HParker as for those last serialization changes we will not need column. That as far as I can tell is only for syntax error reporting after parsing (e.g. in compile.c or IRBuilder.java). If we hit that we throw an exception. Us serialize users can just walk the source to figure out the column in that case. It is a not a hot path activity.

I have an idea for serialization which will cut down on those lines (which can be done in a later PR). The notion is any node which can be a newline will get marked in config.yml (line: true). Any node which needs end line will get (end_line: true). Most nodes do not need the latter so nearly all end lines will get removed. Many line fields will also go away but there will still be plenty.

@eregon I forgot about the conversation about duplicate newlines. If we could appropriately only emit one then that is great but this may be complicated:

def foo
  something_not_newline ? call1 : call2
end

both call1 and call2 will be marked newline but in order to only emit one newline we would need flow analysis. It is possible ternary here is a newline node, but you get the idea. Even if we could cull some lines it would be nice.

[HParker]

I think overall we need to better understand the requirements here before jumping to implementation, how about scheduling a call next week (not Monday for me)?

@eregon sure! I don't know what communication channels you use, but we should set something up.
I haven't met you or @enebo over voice before, so it would be great to chat a bit.

I haven't thought this through completely yet, but right now I am thinking emitting parse errors with line numbers is going to force us to at least know the line number at parse time, so maybe improving parse errors will clarify what bits of the parser need to know line numbers and we can go from there.

Maybe we should try to capture some of the details of this discussion on: #667 to make sure most of the discussion captured there.

[enebo]

@HParker I will update the description in #667 with a more comprehensive description since we uncovered more details since I originally made it.

[enebo]

@HParker ah. I did not realize parse errors already didn't know line number. That is an excellent point.

[eregon]

I wrote above:

Maybe it's fine to limit max line and column to 2^16 (64k), in which case it's the same as two ints. But still likely more memory than varints for offsets. It would also mean the need to have an array of line number to byte offset to be able to extract the relevant substring in the source. So that's one extra indirection for reading from the source. OTOH line&col->offset is O(1) with that table vs O(log n) for the binary search for offset->line&col.

Actually that's not true, while line->offset is O(1) with a table of line to offset, it's not that simple for column, since column is expressed in number of codepoints, not number of bytes. And we can't just have a table of column to offset, that's way too big. So if we store one of the two, offset seems better overall (incidentally, already what Truffle stores and it does the binary search to get line & column info from it).

For Coverage and :line TracePoint, I think we could just compute these line numbers lazily, only when Coverage or :line TracePoint (actually can even compute it lazily on TracePoint#lineno) is used. Then we don't need anything but offsets in the serialized form.

[eregon]

@eregon I forgot about the conversation about duplicate newlines. If we could appropriately only emit one then that is great but this may be complicated:

So I tried this file:

rand < 0.5 ?
  1.itself :
  2.itself

And this is what CRuby (3.2) gives:

$ ruby -rcoverage -e 'Coverage.start; require "./line.rb"; pp Coverage.result'
{"/home/eregon/code/truffleruby-ws/truffleruby/line.rb"=>[1, nil, nil]}

So it considers no node on line 2 or 3, "continuation lines" seem to just be part of the first line where they start (and the nil there means Ruby considers that line "empty" for coverage purpose, which I guess due to no newline-marked node on that line).

And the same for :line TracePoint:

$ ruby -e 'TracePoint.new(:line) { |tp| puts "#{tp.path}:#{tp.lineno}" }.enable; require "./line.rb"' | grep line.rb
/home/eregon/code/truffleruby-ws/truffleruby/line.rb:1

So from this example it sounds like at most one node needs to be marked as "newline" per line.

[HParker]

I am going to close this since I don't think it will merge in its current state, but I found the conversation super helpful and I think we can get closer in another attempt.