Skip to content

gf: cache cache_with_orig decision #48833

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 1 commit into from
Mar 1, 2023
Merged

gf: cache cache_with_orig decision #48833

merged 1 commit into from
Mar 1, 2023

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 28, 2023

Memoizing this can save a lot of repeated effort for queries such as @which eltype(String). Otherwise we repeatedly try to check if eltype(::Type) is a good way to cache that result, even though it never gets better the more we check it.

This can make a large difference for some common queries:

julia> @time Base._methods_by_ftype(Tuple{typeof(eltype), Type{Val{7}}}, -1, Base.get_world_counter())
  0.201521 seconds (523.29 k allocations: 27.287 MiB, 4.41% gc time) # before
  0.000111 seconds (38 allocations: 1.797 KiB) # after
1-element Vector{Any}:
 Core.MethodMatch(Tuple{typeof(eltype), Type{Val{7}}}, svec(), eltype(::Type) @ Base abstractarray.jl:234, true)

@vtjnash
gf: cache cache_with_orig decision
9219f82 
Memoizing this can save a lot of repeated effort for queries such as
`@which eltype(String)`. Otherwise we repeatedly try to check if
`eltype(::Type)` is a good way to cache that result, even though it
never gets better the more we check it.
@vtjnash vtjnash requested a review from topolarity February 28, 2023 21:39
@vtjnash vtjnash added backport 1.9 Change should be backported to release-1.9 latency Latency labels Feb 28, 2023
topolarity
topolarity approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

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

Very nice! 👍

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 28, 2023
@N5N3 N5N3 merged commit bdcd5e2 into master Mar 1, 2023
@N5N3 N5N3 deleted the jn/cache-cache_with_orig branch March 1, 2023 01:09
@N5N3 N5N3 removed the merge me PR is reviewed. Merge when all tests are passing label Mar 1, 2023
@KristofferC KristofferC mentioned this pull request Mar 3, 2023
Merged
50 tasks
KristofferC pushed a commit that referenced this pull request Mar 3, 2023
@vtjnash
gf: cache cache_with_orig decision (#48833)
d5651bc 
Memoizing this can save a lot of repeated effort for queries such as
`@which eltype(String)`. Otherwise we repeatedly try to check if
`eltype(::Type)` is a good way to cache that result, even though it
never gets better the more we check it.

(cherry picked from commit bdcd5e2)
KristofferC pushed a commit that referenced this pull request Mar 3, 2023
@vtjnash
gf: cache cache_with_orig decision (#48833)
b07fb40 
Memoizing this can save a lot of repeated effort for queries such as
`@which eltype(String)`. Otherwise we repeatedly try to check if
`eltype(::Type)` is a good way to cache that result, even though it
never gets better the more we check it.

(cherry picked from commit bdcd5e2)
vchuravy added a commit that referenced this pull request Mar 6, 2023
@vchuravy
fixup! gf: cache cache_with_orig decision (#48833)
fcde569
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@topolarity topolarity topolarity approved these changes

Assignees

No one assigned

Labels

latency Latency

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vtjnash @topolarity @KristofferC @N5N3