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

Improve performance of weeder when type-class-roots = false is set. #172

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

pranaysashank
Copy link
Contributor

For analysing evidence uses we collect evidence uses in,

requestedEvidence :: Map Declaration (Set Name)

In analyseEvidenceUses, we loop over all the names in all the sets of the map, to construct dependency graph after calling getEvidenceTree on the name. However, these names in sets across different declarations are duplicated a lot. In one example in a repo at work, we have 16961625 names in which only 200330 are unique. So now, we instead pre-construct an evidence trees map Map Name [Declaration] for all the unique name and perform a lookup in this map to construct the graph.

In a private repo, the times before this change and after

❯ find . -name '*.hie'  | wc -l
   1097

❯ time result/bin/weeder # weeder from master

real    5m53.707s
user    5m50.350s
sys     0m2.206s

 ❯ time result/bin/weeder # weeder from this branch

real    0m34.008s
user    0m31.716s
sys     0m2.196s

I have a failing test in ./test/Spec/ModuleRoot, but this is failing on master as well.

@ocharles
Copy link
Owner

ocharles commented Aug 5, 2024

Wow, what an incredible speed up! I'll take a look! @ryndubei could you also have a look if you have time?

Copy link
Collaborator

@ryndubei ryndubei left a comment

Choose a reason for hiding this comment

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

Thanks! I don't have a large enough project on hand at the moment that I can quickly test the performance with, but the changes look good to me. I only have a few minor, mostly-style-related comments.

src/Weeder.hs Outdated Show resolved Hide resolved
src/Weeder.hs Outdated Show resolved Hide resolved
src/Weeder.hs Outdated Show resolved Hide resolved
src/Weeder.hs Outdated Show resolved Hide resolved
For analysing evidence uses we collect evidence uses in,

```haskell
requestedEvidence :: Map Declaration (Set Name)
```
In analyseEvidenceUses, we loop over all the names in all the sets of
the map, to construct dependency graph after calling `getEvidenceTree`
on the name. However, these names in sets across different
declarations are duplicated a lot. In one example in a repo at work,
we have 16961625 names in which only 200330 are unique. So now, we
instead pre-construct an evidence trees map `Map Name [Declaration]`
for all the unique name and perform a lookup in this map to construct
the graph.

In a private repo, the times before this change and after

```
❯ find . -name '*.hie'  | wc -l
   1097

❯ time result/bin/weeder # weeder from master

real    5m53.707s
user    5m50.350s
sys     0m2.206s

 ❯ time result/bin/weeder # weeder from this branch

real    0m34.008s
user    0m31.716s
sys     0m2.196s
```
@pranaysashank
Copy link
Contributor Author

@ryndubei I addressed all of your comments

one minor thing I have noticed is there seems to be a slight performance loss with the usage of foldMap & Map.fromSet. Core shows that fromSet doesn't get inlined and foldMap generates recursive code with (++) operators instead as opposed to when we hand roll (concat (map (declMap Map.!) (Set.toList v))) & use Set.foldl'. The same is the case with fromSet (turns out even foldl' has the same O(n) complexity). A quick look at fromSet shows it doesn't have the inline pragma or the usual haskell trick where the recursion is extracted to a separate go function.

Since the performance impact only seems to be at most 1-2 seconds, I think we can prefer code clarity

@pranaysashank
Copy link
Contributor Author

also, why don't we link the executable with -rtsopts? it's useful to change some parameters to figure out how the performance characteristics change or limit the memory used with +RTS -M

@ocharles
Copy link
Owner

Before...

[root@internal:~]# time nix-store -r /nix/store/sc5pfnj0la...-weeder.drv --builders '' --check
real	6m1.956s
user	6m5.595s
sys	0m2.792s

After

[root@internal:~]# time nix-store -r /nix/store/kip37...-weeder.drv --builders '' --check
real	3m5.243s
user	3m11.471s
sys	0m3.609s

Hard to argue with that!

Thanks for you hard work, @pranaysashank!

@ocharles ocharles merged commit 5c34e70 into ocharles:master Aug 10, 2024
1 check passed
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