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

Add recursive findfirst method for tuples. #42423

Merged
merged 6 commits into from
Oct 1, 2021

Conversation

chriselrod
Copy link
Contributor

@chriselrod chriselrod commented Sep 29, 2021

https://discourse.julialang.org/t/why-does-findfirst-t-on-a-tuple-of-typed-only-constant-fold-for-the-first/68893

With this PR:

julia> position(::T) where T = findfirst(==(T), (Int, Float64, Char))
position (generic function with 1 method)

julia> @code_typed position(10)
CodeInfo(
1return 1
) => Int64

julia> @code_typed position(10.0)
CodeInfo(
1return 2
) => Int64

julia> @code_typed position('j')
CodeInfo(
1return 3
) => Int64

julia> @code_typed position(10f0)
CodeInfo(
1return nothing
) => Nothing

@KristofferC
Copy link
Member

Note #42263

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

You can use e.g.:

@test Base.return_types() do
    findfirst(==(2), (1,2,3))
end == Any[Int]

as a test case that is supposed to be improved by this PR.

Also, how about adding findlast also ? It might be enough to add:

function findlast(f::Function, x::Tuple)
    r = findfirst(f, reverse(x))
    return isnothing(r) ? r : length(x) - r + 1
end

base/tuple.jl Outdated
Comment on lines 361 to 365
function findfirst(f::Function, x::Any32)
for i in 1:length(x)
f(x[i]) && return i
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Let's use 4-spaces indents for Base:

Suggested change
function findfirst(f::Function, x::Any32)
for i in 1:length(x)
f(x[i]) && return i
end
end
function findfirst(f::Function, x::Any32)
for i in 1:length(x)
f(x[i]) && return i
end
end

Copy link
Contributor Author

@chriselrod chriselrod Sep 29, 2021

Choose a reason for hiding this comment

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

Done, switched to 4 spaces and renamed to _findfirst_loop following #42263.
Also added the test case.

base/tuple.jl Outdated Show resolved Hide resolved
base/tuple.jl Outdated Show resolved Hide resolved
base/tuple.jl Show resolved Hide resolved
test/tuple.jl Show resolved Hide resolved
aviatesk and others added 4 commits September 30, 2021 05:29
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
@aviatesk
Copy link
Member

Let's see a benchmark result: @nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: failed process: Process(setenv(`git push`; dir="/run/media/system/data/nanosoldier/workdir/NanosoldierReports"), ProcessExited(1)) [1]

Unfortunately, the logs could not be uploaded.
cc @christopher-dG

@aviatesk
Copy link
Member

@vtjnash could you please upload the result ?

@vtjnash
Copy link
Member

vtjnash commented Sep 30, 2021

https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_hash/2976c42_vs_7f26a7f/report.md

yep, looks unchanged to me

@aviatesk
Copy link
Member

Thanks. And yeah, the result looks reasonable and I think this is good to go.

@aviatesk aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Sep 30, 2021
end
return nothing
end
findfirst(f::Function, t::Tuple) = length(t) < 32 ? _findfirst_rec(f, 1, t) : _findfirst_loop(f, t)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should be <= if we want to include the 32 case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with < because Jeff did here: https://github.com/JuliaLang/julia/pull/42263/files

@aviatesk
Copy link
Member

aviatesk commented Oct 1, 2021

All CI failures are known issues.

@aviatesk aviatesk merged commit 7f66828 into JuliaLang:master Oct 1, 2021
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Oct 2, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
These recursive definitions will allow constant-folding for small tuple cases.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
These recursive definitions will allow constant-folding for small tuple cases.
@chriselrod chriselrod deleted the findfirsttuple branch May 1, 2024 18:51
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.

6 participants