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

Stop precomputing chunk sizes #708

Merged
merged 3 commits into from
Aug 19, 2024
Merged

Stop precomputing chunk sizes #708

merged 3 commits into from
Aug 19, 2024

Conversation

KristofferC
Copy link
Collaborator

@KristofferC KristofferC commented Aug 15, 2024

The non-concretely typed vector with chunks prevents constant propagation of the length of a static array. Alternative to #707

This code was used to workaround JuliaLang/julia#29887 but with modern Julia it just inhibits optimizations.

This prevents constant propagation of the length of a static array. Alternative to #707
src/prelude.jl Outdated Show resolved Hide resolved
Co-authored-by: David Widmann <[email protected]>
@ChrisRackauckas
Copy link
Member

To get this actually released, we should do this to the release-0.10 branch?

@KristofferC
Copy link
Collaborator Author

Or at least backport it

@devmotion
Copy link
Member

I ran the benchmark in #373, on the master branch and with this PR:

master

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  328.805 ns (3 allocations: 1.92 KiB)

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  331.407 ns (3 allocations: 1.92 KiB)

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  328.077 ns (3 allocations: 1.92 KiB)

PR #708

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  417.503 ns (3 allocations: 1.92 KiB)

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  406.040 ns (3 allocations: 1.92 KiB)

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  407.500 ns (3 allocations: 1.92 KiB)

Setup

julia> versioninfo()
Julia Version 1.10.4
Commit 48d4fd48430 (2024-06-04 10:41 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: macOS (arm64-apple-darwin22.4.0)
  CPU: 10 × Apple M2 Pro
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, apple-m1)
Threads: 1 default, 0 interactive, 1 GC (on 6 virtual cores)
Environment:
  JULIA_PKG_USE_CLI_GIT = true

@KristofferC
Copy link
Collaborator Author

KristofferC commented Aug 15, 2024

This was an easier way too see the regression:

julia> const x = rand(10)

# master
julia> @btime ForwardDiff.Chunk($x)
  3.792 ns (0 allocations: 0 bytes)
ForwardDiff.Chunk{10}()

# PR
julia> @btime ForwardDiff.Chunk($x)
  55.894 ns (0 allocations: 0 bytes)
ForwardDiff.Chunk{10}()

I made a new commit to try work around that. Could you try again @devmotion?

@devmotion
Copy link
Member

devmotion commented Aug 15, 2024

I reran the benchmark in #373 with the latest commit:

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  358.333 ns (3 allocations: 1.92 KiB)

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  346.847 ns (3 allocations: 1.92 KiB)

julia> @btime ForwardDiff.gradient(rosenbrock, x);
  347.417 ns (3 allocations: 1.92 KiB)

And that's what I get when I benchmark Chunk:

# master
julia> @btime ForwardDiff.Chunk($x)
  3.458 ns (0 allocations: 0 bytes)

# PR
julia> @btime ForwardDiff.Chunk($x)
  3.625 ns (0 allocations: 0 bytes)

@KristofferC
Copy link
Collaborator Author

So would you say this is ok as it is now?

@devmotion
Copy link
Member

Personally, I think the benefits (for type inference and static arrays) are worth the ~5% regression in these benchmarks, even more so since these benchmarks only focus on the construction of Chunk and GradientConfig.

@KristofferC KristofferC merged commit ec74fbc into master Aug 19, 2024
4 checks passed
@KristofferC
Copy link
Collaborator Author

Should backport and release a new version

devmotion added a commit that referenced this pull request Aug 19, 2024

This prevents constant propagation of the length of a static array. Alternative to #707

Co-authored-by: David Widmann <[email protected]>
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

Successfully merging this pull request may close these issues.

3 participants