Make it possible to lazily deserialize DefNode in Loader.java

[eregon]

• TRUFFLERUBY_METRICS_REPS=5 jt metrics time --experimental-options -e0
  For parsing-core:
  before: 0.097 0.099 0.092 0.096
  after:  0.061 0.063 0.066 0.059
  

• Remove extra trailing spaces by using <%-#.

There is still one problem though, which is that Node#newLineFlag is not set inside DefNode when using lazy DefNodes.

• We could keep the MarkNewlinesVisitor as a Loader field and use it in createDefNodeFromSavedPosition(), but MarkNewlinesVisitor holds onto the Source object, which holds on the source byte[]. There seems to be no usage of Source#bytes though (except in findLine() which is replaceable), so then it'd just keep the int[] lineOffsets which seems fine. The specific nodes being flagged might differ with multiple def on the same line, not sure if that matters.
• We could remove MarkNewlinesVisitor and let compilers deal with this entirely on their own.
• We could serialize PM_NODE_FLAG_NEWLINE but that's not equivalent to Node#newLineFlag it's just a potential newline and it would increase serialized size.
• Maybe prism.c could compute & set real newline flags? Not sure if feasible, I forgot the details about this. Would increase serialized size but would probably be a good tradeoff considering it removes a need for an extra tree traversal.

cc @kddnewton @enebo @headius

[kddnewton]

Not really, unfortunately. PM_NODE_FLAG_NEWLINE maps to whether or not a node is a candidate to emit a LINE event in tracepoint. Whether or not it actually does maps to whichever node is reached first on a given line inside the compiler. To recreate it, you'd have to recreate the exact order to CRuby compiler walks the AST, which would be pretty fragile as it can change depending on optimizations.

Honestly, this is probably something that should be codified, because it's super weird to me to have it depend on the compiler. But that's the state it's in. I'm happy to serialize PM_NODE_FLAG_NEWLINE (we already serialize flags, I think it would just be another bit). But that would only help with candidate selection, not actually marking the nodes.

That said, I'm fine with the changes in this PR.

[enebo]

@eregon @kddnewton I advocated in having serialization provide this because the extra walk on a cold runtime seemed like a downside over size. Whether newlines is a flag (like legacy parser) or in a table I don't really have any opinion.

[eregon]

Thank you for the summary.
Currently MarkNewlinesVisitor marks lines in visitor order, i.e. pre-order I think.
It seems to match CRuby fairly well given test/prism/newline_test.rb though there are some differences.

I wonder how hard it would be to do all the newline marking directly in the compilers.
At first sight it sounds possible because the logic is like (MarkNewlinesVisitor & PM_NODE_FLAG_NEWLINE both do that):

• set newline flag on IfNode, UnlessNode and each statement in a StatementsNode
• some nodes override setNewLineFlag to ignore it or recurse to one of their child nodes, that's currently done in MarkNewlinesVisitor + Node subclasses and in CRuby presumably in the compiler
• the flag is only set if there hasn't been a flag already set on that line
  * MarkNewlinesVisitor resets the marked lines on BlockNode & LambdaNode (but not DefNode, sounds odd). I don't recall why but I presume I did it to match CRuby.

I guess there is one way to know for sure, by trying to do it in TruffleRuby or JRuby.
If so, we'd avoid an extra tree traversal and an extra boolean field in Prism Java nodes.

I'm happy to serialize PM_NODE_FLAG_NEWLINE (we already serialize flags, I think it would just be another bit).

From a quick look it's already serialized, but currently omitted for nodes which don't have other flags to save on serialized size. It would be trivial to change but since it's only candidates I think it wouldn't be a big help (would still need an extra traversal or handling while compiling to compute the real line events). Would probably make it easier for compilers though, since that's how it works on CRuby.

[eregon]

@enebo Since you have had and probably still have the logic to mark real newline events from candidate newline flags in JRuby, do you think that's something easy enough to just replicate in every compiler? Would you have a link maybe? (I can also look myself another time, right now it's too late).
Do you think it'd be feasible to do all the newline marking in the compiler, i.e. without any flags from the parser? (see my previous comment)

[enebo]

I should say I like this overall. Using inheritance to opt into lazy also feels like a nice way to do this.

[enebo]

@enebo Since you have had and probably still have the logic to mark real newline events from candidate newline flags in JRuby, do you think that's something easy enough to just replicate in every compiler?

Honestly, my memory is failing me past remembering the discussion. From what I do remember the actual marking was just stmt in the original grammar with some weirdness with begin jumping to first contained node (I just looked at RubyParserBase.newline_node() to confirm). I naively assume prism can do the same thing and just mark a flag on each "statement" node.

[enebo]

I should add I have not looked at any line adjustment in compile.c so there perhaps is more weirdness but hopefully that is not changing the original tree's location of newline on nodes.

[kddnewton]

I do know there's some weirdness where a call node is considered as being on the same line as its message, not its receiver.

[enebo]

@kddnewton Do you think it makes sense to try and normalize this into prism since it would eliminate line numbering bugs across implementations? I am not 100% sure on wrinkles but it seems like it would be helpful (saying this as someone who has to fix a line numbering bug in JRuby for the next point release using the legacy parser).

[kddnewton]

I mean I support that work, but I doubt it would fly with the other folks in CRuby. Also, it is a bit repetitive, because it means that for every tree we would need to walk it once to set the line nodes and then once more to compile it. Right now it just happens once.