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

fix perf issue loadmodel! #2241

Merged
merged 4 commits into from
Apr 25, 2023
Merged

fix perf issue loadmodel! #2241

merged 4 commits into from
Apr 25, 2023

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented Apr 25, 2023

Fix performance issue mentioned in #2239 (comment)

When loading from a flux model into another everything is fine:

using Metalhead, Flux

m1 = ResNet(18)
m2 = ResNet(18)
@time Flux.loadmodel!(m2, m1) # warmup
@time Flux.loadmodel!(m2, m1)
#  0.003388 seconds (23.01 k allocations: 2.157 MiB) # fast on both master and this PR

When loading from a similar struct such as nested named tuples instead (as proposed in #2239) perf on master is very bad:

using Functors

function state(x)
    if Functors.isleaf(x)
        return x
    else
        return map(state, Functors.children(x))
    end
end

s = state(m1)
@time Flux.loadmodel!(m2, s) # warmup
@time Flux.loadmodel!(m2, s)
# 69.507712 seconds (1.06 G allocations: 40.744 GiB, 4.74% gc time) # master
# 0.002409 seconds (6.85 k allocations: 1.109 MiB)                               # this PR

The perf problem was due to huge strings being created at each recursion step as part of error messages.

@theabhirath

Also removed the filter argument, which was not documented nor used in practice.

@theabhirath
Copy link
Member

filter is important for Metalhead. The motivation is described in #2041

src/loading.jl Show resolved Hide resolved
@CarloLucibello
Copy link
Member Author

I'll revert the filter removal. Can we make it documented API?

@darsnack
Copy link
Member

I don't have an issue with that, but I recall that last time there was some bikeshedding and we opted to keep it private. If it's still an issue, no need to let it hold up this PR.

@CarloLucibello CarloLucibello merged commit 7e329f3 into master Apr 25, 2023
rgobbel pushed a commit to rgobbel/Flux.jl that referenced this pull request Apr 25, 2023
* fix perf issue loadmodel!

* reinstate filter

* cleanup

* cleanup
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