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

Don't add an edge to Tuple to the method tables #42

Merged
merged 4 commits into from
Jul 26, 2024

Conversation

Drvi
Copy link
Contributor

@Drvi Drvi commented Jul 24, 2024

We noticed that Tricks.jl were adding an edge to Tuple which was slowing down our package image generation in the verify edges stage, due to expensive method matching; basically doing this:

julia> @time Base._methods_by_ftype(Tuple, 120000, Base.get_world_counter())
1385.862163 seconds (2.49 M allocations: 245.845 MiB, 0.00% gc time)

(our sysimage is quite large making the impact of this very pronounced). I believe this is the issue number #7.

SnoopCompile.@snoopr reported an invalidation of Tuple and pointed to Tricks.static_method_count. After playing with this package for a bit I put together this PR which seems to fix the problem for us, but to be honest, I'm not very confident that what I've done is entirely correct and robust.

Locally the tests were passing as did this toy script:

using Tricks

function foo end
@info Tricks.static_method_count(foo)

foo() = 0
@info Tricks.static_method_count(foo)

foo(x) = 1
@info Tricks.static_method_count(foo)

Base.delete_method(methods(foo)[1])
@info Tricks.static_method_count(foo)

foo(x, y) = 2
@info Tricks.static_method_count(foo)

which produced:

[ Info: 0
[ Info: 1
[ Info: 2
[ Info: 1
[ Info: 2

@coveralls
Copy link

coveralls commented Jul 24, 2024

Pull Request Test Coverage Report for Build 10077702409

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.872%

Totals Coverage Status
Change from base Build 9319814135: 0.0%
Covered Lines: 74
Relevant Lines: 78

💛 - Coveralls

@oxinabox
Copy link
Owner

oxinabox commented Jul 24, 2024

I think this is right.
And I think the difference is related to how parametric types get handled.
Or possibly exclusively to Tuple with its weird varadic type contructor.

Can you also test it on a parametric type constructor.
Both with and without parameters specified.
And possibly add that to the tests?

Maybe also test type pirating the tuple constructor. (I am ok with breaking that)

@Drvi
Copy link
Contributor Author

Drvi commented Jul 24, 2024

Thanks for the swift review, @oxinabox! I added the tests you suggested. The Tuple case is a strange one -- upon adding a new method, two new methods appeared, these were the two (I left the comment in the code):

        #  [6] (::Type{T})(nt::NamedTuple) where T<:Tuple
        #      @ Base namedtuple.jl:198
        #  [7] Tuple(a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, w, x, y, z)
        #      @ Main REPL[5]:1

And it seems that it doesn't happen in Julia 1.3. Any ideas?

@oxinabox
Copy link
Owner

Absolutely wild.
Not a real concern as, noone should be adding method to tuples.
But lets move that test into an issue on this repo and delete it from the test file
Or if it occurs with plain methods openning the issue on julialang/julia.
Its nonsense but i think it should be well behaved nonsense.

Then we can merge this.

Please also bump the version to 0.1.9 so i can tag the release

@Drvi
Copy link
Contributor Author

Drvi commented Jul 24, 2024

Or if it occurs with plain methods openning the issue on julialang/julia.

Yeah this is consistent between Tricks.static_method_count and Base.methods, opened an issue about it JuliaLang/julia#55231

@oxinabox oxinabox merged commit ac53de4 into oxinabox:master Jul 26, 2024
12 checks passed
@Drvi
Copy link
Contributor Author

Drvi commented Jul 26, 2024

Thanks!

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.

3 participants