Skip to content

precompile package callback #341

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 2, 2021
Merged

precompile package callback #341

merged 3 commits into from
May 2, 2021

Conversation

@simeonschaub
Copy link
Member

@simeonschaub simeonschaub commented May 2, 2021

With this PR:

(ChainRulesCore) pkg> precompile
Precompiling project...
  1 dependency successfully precompiled in 2 seconds (1 already precompiled)

julia> @time using ChainRulesCore
  0.082255 seconds (157.13 k allocations: 9.737 MiB, 5.59% compilation time)

Before:

julia> @time using ChainRulesCore
  0.111551 seconds (443.47 k allocations: 25.618 MiB, 3.87% compilation time)

Removing the hook alltogether:

julia> @time using ChainRulesCore
  0.033357 seconds (36.37 k allocations: 2.399 MiB, 17.75% compilation time)

That's still more overhead than I'd like, so we should still discuss
whether we could avoid these hooks, but it does at least improve the current
situation somewhat.

Ref #340

@simeonschaub
precompile package callback
f3939cc 
With this PR:
```
(ChainRulesCore) pkg> precompile
Precompiling project...
  1 dependency successfully precompiled in 2 seconds (1 already precompiled)

julia> @time using ChainRulesCore
  0.082255 seconds (157.13 k allocations: 9.737 MiB, 5.59% compilation time)
```

Before:
```
julia> @time using ChainRulesCore
  0.111551 seconds (443.47 k allocations: 25.618 MiB, 3.87% compilation time)
```

Removing the hook alltogether:
```
julia> @time using ChainRulesCore
  0.033357 seconds (36.37 k allocations: 2.399 MiB, 17.75% compilation time)
```

That's still more overhead than I'd like, so we should still discuss
whether we could avoid these hooks, but it does at least improve the current
situation somewhat.

Ref #340
@simeonschaub simeonschaub requested a review from oxinabox May 2, 2021 16:14
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2021 •
edited
Loading

Codecov Report

Merging #341 (bea0985) into master (fe69977) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   90.00%   90.01%   +0.01%     
==========================================
  Files          14       14              
  Lines         540      541       +1     
==========================================
+ Hits          486      487       +1     
  Misses         54       54
Impacted Files Coverage Δ
src/ChainRulesCore.jl 100.00% <ø> (ø)
src/ruleset_loading.jl 95.83% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe69977...bea0985. Read the comment docs.

@oxinabox
Copy link
Member

oxinabox commented May 2, 2021

Nice.
We can talk about removing this in #340 (and as to if that is breaking)

But for now this seems good

oxinabox
oxinabox approved these changes May 2, 2021
View reviewed changes
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Don't for get to bump the version and tag a release

src/ruleset_loading.jl Outdated
end
end

precompile(_package_hook, (Base.PkgId,))
Copy link
Member

@oxinabox oxinabox May 2, 2021

Choose a reason for hiding this comment

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

Can we put this in a seperate file?
precompile.jl

We should probably be precompiling a bunch of things

Copy link
Member Author

@simeonschaub simeonschaub May 2, 2021

Choose a reason for hiding this comment

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

Ok, will do

@KristofferC
Copy link

KristofferC commented May 2, 2021 •
edited
Loading

What's the timing on putting these stuff inside its own module and setting @eval Base.Experimental.@compiler_options compile=min optimize=0 infer=false?

@simeonschaub
Copy link
Member Author

simeonschaub commented May 2, 2021 •
edited
Loading

What's the timing on putting these stuff inside its own module and setting @eval Base.Experimental.@compiler_options compile=min optimize=0 infer=false?

julia> @time using ChainRulesCore
  0.104489 seconds (97.03 k allocations: 5.980 MiB, 3.87% compilation time)

So it takes a little longer than with this PR, but allocations go down.

@KristofferC
Copy link

KristofferC commented May 2, 2021

Hm, I wonder what still takes so much time...

@simeonschaub
Copy link
Member Author

simeonschaub commented May 2, 2021

Not sure, profiling wasn't very conclusive to me. I am going to merge this for now, since it's a definite improvement over the current state.

@simeonschaub simeonschaub merged commit 5f1613f into master May 2, 2021
@simeonschaub simeonschaub deleted the sds/precompile_hook branch May 2, 2021 17:16
@oxinabox oxinabox mentioned this pull request May 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oxinabox oxinabox oxinabox approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@simeonschaub @codecov-commenter @oxinabox @KristofferC