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

Move shape information of RArray to field #58

Merged
merged 10 commits into from
Aug 14, 2024
Merged

Conversation

mofeing
Copy link
Collaborator

@mofeing mofeing commented Aug 4, 2024

Discussed in #52

@wsmoses Unfortunately, these commits are tangled with the move to the Tracing.jl file.

Changes

  • Shape information of an RArray will no longer be contained in a type parameter, but in a field.
  • Move tracing-related code to Tracing.jl file.

Copy link
Member

@wsmoses wsmoses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally seems reasonable to me -- but would you be able to add the ci for benchmarking so we can test the compile time impact here?

@mofeing
Copy link
Collaborator Author

mofeing commented Aug 6, 2024

This might be hard because what needs to be measured is only the compilation time of the first execution.

Also, how should the info be displayed? Just print the timings?

@wsmoses
Copy link
Member

wsmoses commented Aug 6, 2024

@mofeing so it should be fairly easy to do this: https://github.com/EnzymeAD/Enzyme.jl/blob/main/benchmark/benchmarks.jl via https://github.com/EnzymeAD/Enzyme.jl/blob/main/.github/workflows/benchmark_pr.yml

which puts automatically a comment in the PR with the overhead change

@wsmoses
Copy link
Member

wsmoses commented Aug 13, 2024

@mofeing can you rebase this and see if benchmarking runs?

@mofeing mofeing force-pushed the refactor/shape-rarray branch from c5275a8 to 13b5887 Compare August 13, 2024 21:20
@mofeing
Copy link
Collaborator Author

mofeing commented Aug 13, 2024

I'm on it!

@mofeing
Copy link
Collaborator Author

mofeing commented Aug 13, 2024

The workflow seems to work but it's BenchmarkTools which is giving me a small headache

@mofeing mofeing force-pushed the refactor/shape-rarray branch from 5eccc66 to 15469d7 Compare August 14, 2024 09:07
@mofeing mofeing force-pushed the refactor/shape-rarray branch from 15469d7 to 6e1c44d Compare August 14, 2024 10:06
Copy link
Contributor

Benchmark Results

main 6e1c44d... main/6e1c44df810804...
comptime/basics/2D sum 3.45 s 3.21 s 1.07
comptime/basics/Basic cos 0.19 ± 0.02 s 0.189 ± 0.023 s 1.01
comptime/basics/Basic grad cos 6.85 s 6.61 s 1.04
time_to_load 1.36 ± 0.037 s 1.46 ± 0.011 s 0.933

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@mofeing
Copy link
Collaborator Author

mofeing commented Aug 14, 2024

Finally got it working.

@wsmoses I just added some basic benchmarks. Don't know if you want some more?

@mofeing mofeing merged commit 5e1c02a into main Aug 14, 2024
10 of 15 checks passed
@mofeing mofeing deleted the refactor/shape-rarray branch August 14, 2024 16:50
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.

2 participants