Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected allocation in Julia 1.9/1.10 #49241

Closed
dpinol opened this issue Apr 3, 2023 · 9 comments
Closed

Unexpected allocation in Julia 1.9/1.10 #49241

dpinol opened this issue Apr 3, 2023 · 9 comments
Assignees

Comments

@dpinol
Copy link

dpinol commented Apr 3, 2023

The code below does not allocate in Julia 1.8, but it allocates 16 bytes in Julia 1.9 and 48 in Julia 1.10.

struct S
    a::Vector{Int}
end

const i = S(Int[])

function f(a::S)::Int
    isempty(a.a) && return 1
    return error("we")
end
@allocated(f(i))
16
versioninfo()
Julia Version 1.9.0-rc2
Commit 72aec423c2a (2023-04-01 10:41 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 12 × Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, skylake)
  Threads: 1 on 12 virtual core
@maleadt
Copy link
Member

maleadt commented Apr 4, 2023

bbdee0b216385a0c519450a7f6a0cdbada6cea5d is the first bad commit
commit bbdee0b216385a0c519450a7f6a0cdbada6cea5d
Author: Keno Fischer <keno@juliacomputing.com>
Date:   Fri Oct 28 18:23:12 2022 -0400

    inlining: Don't inline concrete-eval'ed calls whose result was too large (#47371)

    This undoes some of the choices made in #47283 and #47305.
    As Shuhei correctly pointed out, even with the restriction to
    `nothrow`, adding the extra flags on the inlined statements
    results in incorrect IR. Also, my bigger motivating test case
    turns out to be insufficiently optimized without the effect_free
    flags (which I removed in the final revision of #47305).
    I think for the time being, the best course of action here
    is to just stop inlining concrete-eval'ed
    calls entirely. The const result is available for inference,
    so in most cases the call will get deleted. If there's an
    important case we care about where this does not happen,
    we should take a look at that separately.

ref #47371

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 4, 2023

Looks like we are failing to optimize the getproperty call when inlining f(i), but f(i) itself would be fine if called:

    │ @ timing.jl:428 within `macro expansion`
5 ──│ %16 = Main.i::S
│   │┌ @ REPL[3]:2 within `f`
│   ││ %17 = invoke Base.getproperty(%16::S, :a::Symbol)::Vector{Int64}

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 4, 2023

(note that this was explicitly the goal of #47371, but that result seems a bit questionable in this context, since it means we may get much worse IR after the inlining pass if constant-prop was attempted, since running that now blocks inlining)

@Keno
Copy link
Member

Keno commented May 10, 2023

I looked into this, but couldn't reproduce the regression on current master with the posted reproducer.

@KristofferC
Copy link
Sponsor Member

#49697 also claims that #47371 is responsible for some regression.

@dpinol
Copy link
Author

dpinol commented May 10, 2023

Hi,
I just updated master julia and I still can reproduce.

thanks

git pull && nice ionice make -j3
Already up to date.
➜  julia git:(master) usr/bin/julia 
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.10.0-DEV.1255 (2023-05-10)
 _/ |\__'_|_|_|\__'_|  |  Commit e204e200df (0 days old master)
|__/                   |

julia> struct S
           a::Vector{Int}
       end

julia> const i = S(Int[])
S(Int64[])

julia> function f(a::S)::Int
           isempty(a.a) && return 1
           return error("we")
       end
f (generic function with 1 method)

julia> @allocated(f(i))
16

julia> @allocated(f(i))
16

@dpinol
Copy link
Author

dpinol commented Jun 7, 2023

Hi,

  1. Indeed it looks related to 1.8.5 -> 1.9 regression in number of allocations from getproperty #49697 since the allocation goes away when S instance is not a global const.
struct S
  a::Vector{Int}
end
function f(a::S)::Int
  isempty(a.a) && return 1
  return error("we")
end
function g()
  t=S(Int[])
  @allocated f(t)
end
  1. This workaround for using a global const does not work
function g()
  t=i
  @allocated f(t)
end; g()
16
  1. This workaround for using a global const does work
function g(t=i)
  @allocated f(t)
end; g()
0

thanks

@nsajko
Copy link
Contributor

nsajko commented Jul 4, 2023

Note that a workaround is using a getter for the a property:

struct S
    a::Vector{Int}
end

const i = S(Int[])

g(a::S) = a.a

function f(a::S)::Int
    isempty(g(a)) && return 1
    error("we")
end

@allocated f(i)
@allocated f(i)  # zero

These issues seem possibly related: #49697 #50073 #50317

Also discussed on Discourse: https://discourse.julialang.org/t/indexing-on-a-pseudo-dimension/101119/10

@gbaraldi
Copy link
Member

This is fixed by #50444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants