Allocation regression: global const getfield allocates in some cases

[NHDaly]

Here's a small regression that is showing up pretty large in the RAI codebase (we have a global lock in our database pager that's way slower now).

Somehow accessing the fields of a global constant has become type unstable in some cases.

struct Wrapper
    lock::ReentrantLock
end

Base.lock(f::Function, m::Wrapper) = Base.lock(f, m.lock)
Base.lock(m::Wrapper) = Base.lock(m.lock)
Base.unlock(m::Wrapper) = Base.unlock(m.lock)

const MONITOR = Wrapper(ReentrantLock())

function foo()
    Base.@lock MONITOR begin
        return 2+2
    end
end
foo()
@time foo()  # 0 allocs on 1.8, 1 alloc on 1.9

The allocation only shows up when referencing the global const variable. It disappears if you pass it in as an argument:

function bar(x)
    Base.@lock x begin
        return 2+2
    end
end
bar(MONITOR)
@time bar(MONITOR) # 0 allocs on 1.8, 0 allocs on 1.9

This might be related to #50073, but i'm not sure.

[KristofferC]

Bisected to bbdee0b, cc @Keno

[NHDaly]

Thanks @KristofferC. It looks like the regressions were identified last November:
bbdee0b#commitcomment-89854661

Did we decide we will just accept these for 1.9? Is there anything that can be done to address them? They are currently having quite significant impact in our upgrade attempts.

[KristofferC]

Did we decide we will just accept these for 1.9?

Not sure what you mean, 1.9 is already out.

Is there anything that can be done to address them?

Fix it and backport the fix I would assume.

[NHDaly]

Not sure what you mean, 1.9 is already out.

I meant: back in November, when this issue was identified, there weren't any follow up comments. So did we explicitly decide that the regression was acceptable for the (then) upcoming release? Or was it accidentally missed?

I'm asking because if it was just accidentally missed, there's still hope that it's an easy fix. If it was explicitly decided that we would live with it, then maybe there are reasons it will be hard to fix for a 1.9.2.

Sorry for my sloppy / confusing language. :)

[Keno]

Somebody just needs to decide what it should do and what the inlining rules are.

[Keno]

To expand on that, the issue here is that the compiler improved, so we now know the constant result of the getproperty.
However, because it's mutable, we refuse to use the result for inlining (but we do use it for the rest of inference). So there's basically three choices here:

1. Allow mutable values to be inlineable constants, by tweaking the code here (but is this ok or will that blow up the serializer?): https://github.com/JuliaLang/julia/blob/master/base/compiler/utilities.jl#L88
2. Allow some functions to inline even if their result is not inlineable (tricky, because you risk losing type informations and it's not profitable in general, so how do you draw the line for the predicate)
3. Fix the generated code get getproperty to get rid of the useless allocation (reasonably straightforward, but probably not sure general).

[kpamnany]

The third choice seems like the easiest way forward here, and additionally, seems like it will address our problem. Can we request that fix as it's beyond my understanding?

[KristofferC]

I meant: back in November, when this issue was identified, there weren't any follow up comments. So did we explicitly decide that the regression was acceptable for the (then) upcoming release? Or was it accidentally missed?

Looking at bbdee0b#commitcomment-89859406 it seems it got missed and never got "promoted" from a comment to a milestoned issue.

[NHDaly]

Thanks Kristoffer. 👍

+1 that it would be great to get this resolved. It's currently blocking our upgrade since, like I said, it's making all locks+unlocks in a core component (our pager) noticeably slower.

We're seeing like a 20%+ regression in some workloads I think?