Some compiler optimizations

[asterite]

Some optimizations from musings in #8527 (comment)

The first two commits are self-explanatory. The third commits has a big commit message explaining the description of the problem. After reading that you can understand how it applies to this particular example:

require "spec"

macro assert(call, file = __FILE__, line = __LINE__)
  {% obj = call.receiver %}
  {% name = call.name %}
  %obj = {{obj}}
  %call = %obj.{{name}}
  unless %call
    fail "Expected #{%obj} to be {{name[0...-1]}}"
  end
end

describe "Foo" do
  {% for i in 0..12_000 %}
    it "bar {{i}}" do
      a = [1, 2, 3]
      assert a.empty?
    end
  {% end %}
end

In the above code the assert macro will define the local variables %obj and %call which will actually expand to variables named something like __temp1 and so on, and they'll all be different. Because of the above compiler bug, all these local variables will be seen in each assert macro invocation. So the first assert expansion can only see the a local variable. The second one will see that plus the two local variables introduced by the previous macro invocation (which shouldn't be seen, but that's the bug the third commit is fixing). On each invocation a Set is created with these local vars. That has something like quadratic complexity which explains why the above code was taking 12 seconds. Now it takes 0.4s (well, at least the "main" semantic phase).

I always thought macro expansion was slow, but apparently it's not. Or at least not that slow.

Once we merge this PR we can see if #3095 is still slow for the snippet I found it to be slow.

[asterite]

Well, actually, it seems power_assert is still slow to compile but because it generates a ton of code. I think the assert could be made simpler, I'll see.

[jkthorne]

Do you have any benchmarks for this?

[vlazar]

Well, actually, it seems power_assert is still slow to compile but because it generates a ton of code. I think the assert could be made simpler, I'll see.

Maybe this simpler generated assert code proposed by RX14 might help

[straight-shoota]

I suppose there's a mistake in the example code from the third commit? Or else I just don't get it.

[asterite]

Ah, yes, there's a mistake. It should be puts id(a). I'll fix it later.

The benchmark is the snippet with that assert. Compile it with the existing compiler and with a compiler with this PR and you'll see a difference (of about 12 seconds less on my machine).

[yxhuvud]

Would there be any difference if this was (old_vars_size - @vars.size).times do ? I doubt it is noticeable but the method is called a lot of times, so who knows?

I wish we had an ordered map type, then we could have just nuked the last n entries in a more efficient manner.

EDIT: Hmm. Isn't the insertion order kept track of? Perhaps it is possible to do bulk operations. 🤔

/probably not worth the trouble though

[asterite]

All excellent observations. Yeah, Hash is ordered and so is Set so we could nuke the last entries, though because it's a Hash we still need to find the key hash to see in which bucket it's located, clear it and so on, so probably the same performance in the end.

[RX14]

❤️

This is amazing, thank you!!

I think there's likely as lot of low-hanging performance fruit in the compiler which could push out the scaling problem until it's a company with more resources's problem :P

[asterite]

I'd merge but there's no newer "milestone", so I guess we can wait until that exists... or we create one now? Can we merge before 0.32.0 is released?

[bcardiff]

Merging it for 0.32 is fine on my end. As long as there are no breaking changes :-)

[asterite]

Cool, let's do it!