Inline ASTNode bindings dependencies and observers
#15098
Conversation
This allows big savings in number and bytes allocated since most ASTNodes will have on average less than 2 elements in each of these lists.
|
Damn... I wished I saw this before I even started 😬 The CI is failing anyway, and maybe the reason is the same as the last time. Will investigate further. |
|
Ah, I assumed you were aware of that. 🙈 Anyway the issue here must be something relatively obvious. I presume it shouldn't be hard to fix. The interpreter failure was more subtle. |
No worries. It was pretty straightforward to implement. I know that you mentioned it, but it didn't occur to me that there was a trail to follow.
Not sure it's obvious 😬 So far, I spent more time trying to figure out what's broken than actually implementing the optimisation. The good news is that this doesn't reproduce #12769. |
|
Turning |
| while true | ||
| dependencies = node.dependencies.select { |dep| dep.type? && dep.type.includes_type?(owner) && !visited.includes?(dep) } |
There was a problem hiding this comment.
thought: I'm wondering if we could avoid the array allocation here (and maybe in similar locations). We're only interested in the first matching item, so Enumerable#find(&) should be sufficient.
This is an bit of a different change and maybe should go into a separate PR. But it could also fit in the scope here.
There was a problem hiding this comment.
Indeed, I'm pretty sure the entire while block can be replaced with just the following:
while node = node.dependencies.find { |dep| dep.type? && dep.type.includes_type?(owner) && !visited.includes?(dep) }
nil_reason = node.nil_reason if node.is_a?(MetaTypeVar)
owner_trace << node if node
visited.add node
endI pushed a commit to my fork, that way it's running CI immediately so we can confirm this works. oprypin@4203133
Or well, the CI got cancelled and overridden by the next commit on top of it - see my next comment.
There was a problem hiding this comment.
Nice, thank you! Looks good, I'll pull both your commits.
| while deps = node.dependencies | ||
| dependencies = deps.select { |dep| dep.type? && dep.type.includes_type?(owner) && !visited.includes?(dep) } | ||
| while true | ||
| dependencies = node.dependencies.select { |dep| dep.type? && dep.type.includes_type?(owner) && !visited.includes?(dep) } | ||
| if dependencies.size > 0 |
There was a problem hiding this comment.
suggestion: Use !dependencies.empty? here, it improves efficiency by not having to iterate the list.
| def size | ||
| size = 0 | ||
| size += 1 unless @first.nil? | ||
| size += 1 unless @second.nil? | ||
| if tail = @tail | ||
| size += tail.size | ||
| end | ||
| size | ||
| end |
There was a problem hiding this comment.
suggestion: We should keep the implementation of #size. The parent implementation of Enumerable#size iterates all elements which would be less efficient than adding the known tail size.
| while true | ||
| dependencies = node.dependencies.select { |dep| dep.type? && dep.type.includes_type?(owner) && !visited.includes?(dep) } |
There was a problem hiding this comment.
Indeed, I'm pretty sure the entire while block can be replaced with just the following:
while node = node.dependencies.find { |dep| dep.type? && dep.type.includes_type?(owner) && !visited.includes?(dep) }
nil_reason = node.nil_reason if node.is_a?(MetaTypeVar)
owner_trace << node if node
visited.add node
endI pushed a commit to my fork, that way it's running CI immediately so we can confirm this works. oprypin@4203133
Or well, the CI got cancelled and overridden by the next commit on top of it - see my next comment.
| def each(& : ASTNode ->) | ||
| if first = @first | ||
| yield first | ||
| end | ||
| if second = @second | ||
| yield second | ||
| end | ||
| if tail = @tail | ||
| tail.each { |node| yield node } | ||
| end | ||
| end | ||
|
|
||
| def push(node : ASTNode) : self | ||
| if @first.nil? | ||
| @first = node | ||
| elsif @second.nil? | ||
| @second = node | ||
| else | ||
| tail = @tail ||= Array(ASTNode).new | ||
| tail.push(node) | ||
| end | ||
| self | ||
| end | ||
|
|
||
| def reject!(& : ASTNode ->) : self | ||
| if first = @first | ||
| if yield first | ||
| @first = nil | ||
| end | ||
| end | ||
| if second = @second | ||
| if yield second | ||
| @second = nil | ||
| end | ||
| end | ||
| if tail = @tail | ||
| tail.reject! { |node| yield node } | ||
| end | ||
| self | ||
| end |
There was a problem hiding this comment.
I converted this collection so that it never has "holes". reject! needs to have a much more complicated implementation (so the code is harder to trust), but in turn everything else can have an early return. Also it preserves push order.
Feel free to ignore this, I just wanted to have fun :D
| def each(& : ASTNode ->) | |
| if first = @first | |
| yield first | |
| end | |
| if second = @second | |
| yield second | |
| end | |
| if tail = @tail | |
| tail.each { |node| yield node } | |
| end | |
| end | |
| def push(node : ASTNode) : self | |
| if @first.nil? | |
| @first = node | |
| elsif @second.nil? | |
| @second = node | |
| else | |
| tail = @tail ||= Array(ASTNode).new | |
| tail.push(node) | |
| end | |
| self | |
| end | |
| def reject!(& : ASTNode ->) : self | |
| if first = @first | |
| if yield first | |
| @first = nil | |
| end | |
| end | |
| if second = @second | |
| if yield second | |
| @second = nil | |
| end | |
| end | |
| if tail = @tail | |
| tail.reject! { |node| yield node } | |
| end | |
| self | |
| end | |
| def each(& : ASTNode ->) | |
| if first = @first | |
| yield first | |
| if second = @second | |
| yield second | |
| if tail = @tail | |
| tail.each { |node| yield node } | |
| end | |
| end | |
| end | |
| end | |
| def size | |
| if @first.nil? | |
| 0 | |
| elsif @second.nil? | |
| 1 | |
| elsif (tail = @tail).nil? | |
| 2 | |
| else | |
| 2 + tail.size | |
| end | |
| end | |
| def push(node : ASTNode) : self | |
| if @first.nil? | |
| @first = node | |
| elsif @second.nil? | |
| @second = node | |
| elsif (tail = @tail).nil? | |
| @tail = [node] of ASTNode | |
| else | |
| tail.push(node) | |
| end | |
| self | |
| end | |
| def reject!(& : ASTNode ->) : self | |
| if first = @first | |
| if second = @second | |
| if tail = @tail | |
| tail.reject! { |node| yield node } | |
| end | |
| if yield second | |
| @second = tail.try &.shift? | |
| end | |
| end | |
| if yield first | |
| @first = @second | |
| @second = tail.try &.shift? | |
| end | |
| end | |
| self | |
| end |
I pushed a commit to my fork, that way it's running CI immediately so we can confirm this works. oprypin@f967ade?diff=split
There was a problem hiding this comment.
Nice fun. I believe #each can actually be shortened a bit more:
def each(& : ASTNode ->)
yield @first || return
yield @second || return
@tail.try(&.each { |node| yield node })
endThere was a problem hiding this comment.
I incorporated this change as well, thanks!
Co-Authored-By: Johannes Müller <[email protected]>
Every
ASTNodecontains two arrays used for type inference and checking: dependencies and observers. By default, these are created lazily, but most active (ie. effectively typed)ASTNodes end up creating them. Furthermore, on average both these lists contain less than 2 elements each.This PR replaces the both
Array(ASTNode)?references inASTNodeby inlined structs that can hold two elements and a tail array for the cases where more links are needed. This reduces number of allocations, bytes allocated, number of instructions executed and running time.Some numbers from compiling the Crystal compiler itself without running codegen (since type binding occurs in the semantic phase anyway).
Running time (measured with hyperfine running with
GC_DONT_GC=1): ~6% reductionBefore:
After:
Memory (as reported by the compiler itself, with GC): ~9.6% reduction
Before:
After:
Callgrind stats:
GC_malloc_kindcall count: 35,161,616 -> 25,684,997 (~27% reduction)