Repository navigation
Keep coverage source positions off AST nodes when Coverage isn't in use - #9795
Merged
Merged
Conversation
sferik
force-pushed
the
coverage-node-positions
branch
from
October 10, 2026 10:25
62e4507 to
89efeb1
Compare
Member
|
Yeah good stuff, this is the result that we wanted. The agent is a bit wrong here in that the reference will be a "compressed pointer" on all modern JVMs, so 4 bytes instead of 8. Worth us doing another audit of all nodes to find more shrinking opportunities, but adding 4 bytes across the board to enable full coverage support is pretty manageable. I'll review and merge. |
Branch and method coverage report where things are in the source, so the parser records a span on every node, and more on some: where the else of a conditional is, the parentheses around an expression, the statement of a modifier loop. All of it sat in fields of the nodes, which made every node 16 bytes larger, and a conditional or a loop up to 40, whether or not Coverage was in use. An AST is large in almost any application and much of it lives as long as the code does. As suggested in jruby#9751 (comment), a node now refers to a record holding its positions, and the parser only records them when Coverage has a use for them. Any other parse leaves that one reference null. - NodeSpan holds a node's span and its parentheses. Each production handing a node along widens its span, which replaces the record. Parsing CRuby 4.0's standard library with positions recorded takes about 5% more CPU time that way than with a mutable holder (0.200s against 0.191s, the median of seven interleaved rounds, and 0.185s with no positions recorded). - What a class of node keeps besides its span goes in a record of its own inside the NodeSpan, so it takes no field of the node: a SourcePosition for the else of a when or in clause, the end of a safe navigation and the arrow of a lambda, a SourceSpan for the body of a modifier loop, and a Branch for a conditional. - Branch coverage needs all of these. Method coverage only needs to know where definitions and blocks start and end, so that is all a parse records for it. - What decides the code that is compiled stays on the node, as flags: that an expression is parenthesized, that a conditional is a modifier, and what MRI folds of a literal condition. The first shares a byte with the flag for a newline that is only there for backtraces, rather than each being a boolean. That leaves the last byte of a node's own 24 free, and the JVM puts a flag of the node's class there, such as whether a call is lazy, instead of starting another 8 bytes for it. - A block and a method definition have their end line in a field of their own again, since every parse needs it. A parse decides what to record when it starts. If branch coverage starts before it ends, the file is measured with what was recorded, and so reports no branches. Instance sizes in bytes (compressed oops), before the coverage work, on the branch this one is based on, and now: Node 24 -> 40 -> 24 CallNode 40 -> 72 -> 40 FCallNode 32 -> 56 -> 40 IfNode 32 -> 80 -> 40 WhileNode 32 -> 72 -> 32 WhenNode 32 -> 64 -> 40 IterNode 40 -> 56 -> 40 LambdaNode 40 -> 56 -> 40 DefnNode 40 -> 64 -> 48 LocalVarNode 32 -> 48 -> 32 FixnumNode 32 -> 48 -> 32 SymbolNode 24 -> 48 -> 32 The reference takes 4 bytes, being a compressed pointer, and objects are aligned to 8. So a class either had the padding to take it or grew by 8: of the 128 classes of node, 64 are the size they were before coverage and 64 are 8 bytes over. With positions recorded, the branches and lines Coverage reports for the 727 files of CRuby 4.0's standard library are what they were, and Coverage.line_stub gives the same result for each of them whether or not positions are recorded. Tests check which nodes carry positions in each coverage mode, so that a parse without Coverage keeps none.
sferik
force-pushed
the
coverage-node-positions
branch
from
October 11, 2026 13:34
89efeb1 to
b8e6de8
Compare
Contributor
Author
|
@headius I’ve updated the commit message and PR description to say the reference is a 4-byte compressed pointer. That also showed a little more room: packing two of |
Member
|
Much cleaner, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the follow-up @enebo described in #9751 (comment). The branch and method coverage work added a source span to every node, plus a few extra positions on conditionals, loops,
when/inclauses and safe-navigation calls. All of that lived in fields, so every node got 16 bytes bigger and anIfNodewent from 32 to 80, whether or not anyone had calledCoverage.start.With this change a node holds a single reference to a record with its positions, and the parser only fills it in when Coverage needs it. Every other parse leaves it null.
Sizes in bytes per instance (compressed oops), measured from field offsets:
masterNodeCallNodeFCallNodeIfNodeWhileNodeWhenNodeIterNodeLambdaNodeDefnNodeLocalVarNodeFixnumNodeSymbolNodeThe reference is a compressed pointer, so it costs 4 bytes, and objects are aligned to 8. A class either had the padding to take it or grew by 8: of the 128 node classes, half are the size they were in 10.1.2.0 and half are 8 bytes over.
Nodealso keeps two flags the coverage work added (a backtrace-only newline, and parenthesized) in one byte now instead of a boolean each. That leaves the last byte of its 24 free, and the JVM puts a subclass's own flag there. This brought nine classes back to their 10.1.2.0 size,CallNode,AttrAssignNode,ArgumentNodeandWhileNodeamong them.Cc: @headius @enebo