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 a Base.tail method for nonempty tuples #52035

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Nov 5, 2023

I have some code that sorts tuples using recursion. When the tuple length gets big (over around 65), suddenly I get lots of allocations in my code, when there should be none. Using @profview_allocs shows that the allocations happen in Base.tail. This simple implementation fixes that.

@nsajko
Copy link
Contributor Author

nsajko commented Nov 5, 2023

If this is good to go, perhaps it would also make sense to reimplement argtail in terms of tail.

@nsajko nsajko changed the title reimplement Base.tail add a Base.tail method for nonempty tuples Nov 5, 2023
@nandoconde
Copy link
Contributor

Not that I am an expert at all, but would not using Any prevent any kind of return type inference here?

@nsajko
Copy link
Contributor Author

nsajko commented Nov 6, 2023

The Any, specifically should have no ill effect. Regarding the question of "does this change affect return type inference?", the answer seems to be "no". The following is the worst case for return type inference, I think, and it's perfect both before and after:

julia> struct S{p}
         v::Int
       end

julia> t = ntuple((i -> S{i}(i)), Val(1000));

julia> using Test

julia> @inferred Base.tail(t);  # no error!

I have some code that sorts tuples using recursion. When the tuple
length gets big (over around 65), suddenly I get lots of allocations in
my code, when there should be none. Using `@profview_allocs` shows that
the allocations happen in `Base.tail`. This simple implementation fixes
that.
@nsajko
Copy link
Contributor Author

nsajko commented Nov 9, 2023

Indentation fixed (four spaces).

@ViralBShah
Copy link
Member

Would be good to get a review and merge.

@jariji
Copy link
Contributor

jariji commented Apr 10, 2024

Perhaps some testing, and a comment or two about why it's written that way?

@CameronBieganek
Copy link
Contributor

Should the tail(x::Tuple) = argtail(x...) method in base/essentials.jl be deleted? Or is it needed for bootstrapping or something like that?

(The empty tuple case is of course handled by the tail(::Tuple{}) method in base/essentials.jl.)

@martinholters
Copy link
Member

Seems like something like code_typed(Base.tail, Tuple{Tuple{Int,Int,Int,Vararg{Int}}}) is inferred better with the current implementation than with the proposed one (Tuple{Int,Int,Vararg{Int}} vs. just Tuple).

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.

None yet

6 participants