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

ExtractDependencies uses more efficient caching #18403

Merged
merged 9 commits into from
Sep 22, 2023

Conversation

bishabosha
Copy link
Member

alternative to #18219

reduces allocations in the phase by 77% (on scala3-compiler-bootstrapped), allocations in the whole compilation pipeline are reduced from about 12% to 4% contributed by this phase.

Reduced allocations come from:

  • reusing a single set for the seen cache.
  • use linear probe maps (dotty.tools.dotc.util) avoiding allocation of nodes
  • no ClassDependency allocation each time a dependency exists (many are likely duplicated)

performance is also improved due to less expensive hashing (EqHashMap), and introduction of EqHashSet, also reducing number of times we hash (add method on HashSet, and optimised getOrElseUpdate). Probably also from reduced heap pressure of allocating.

heap before:

Screenshot 2023-08-15 at 17 34 39

heap after:

Screenshot 2023-08-15 at 17 35 04

As you can see now the majority of allocation is by Zinc (MappedFileConverter)

@bishabosha
Copy link
Member Author

test performance with #sbt please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/18403/ to see the changes.

Benchmarks is based on merging with main (ca6a80e)

@bishabosha
Copy link
Member Author

test performance with #sbt please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

v = v1
update(key, v1)
v.uncheckedNN
// created by blending lookup and update, avoid having to recompute hash and probe

Choose a reason for hiding this comment

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

Sorry if missed something, but it looks like HashMap and EqHashMap both inherit from the GenericHashMap and override this method with exactly the same code? Is there a reason for this, or could the duplication be reduced? 🙂

Copy link
Member Author

@bishabosha bishabosha Aug 15, 2023

Choose a reason for hiding this comment

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

This was done just because all the other methods are duplicated in EqHashMap, so this one calls the overrides - one could compare but I would assume the original reason still stands

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/18403/ to see the changes.

Benchmarks is based on merging with main (ca6a80e)

@bishabosha bishabosha added itype:performance backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Aug 16, 2023
@bishabosha bishabosha assigned sjrd and unassigned Kordyjan Sep 22, 2023
@bishabosha
Copy link
Member Author

@sjrd asking for your review now 🎖️

compiler/src/dotty/tools/dotc/util/EqHashSet.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/util/GenericHashSet.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/util/GenericHashSet.scala Outdated Show resolved Hide resolved
@bishabosha bishabosha merged commit 48a7871 into scala:main Sep 22, 2023
16 checks passed
@bishabosha bishabosha deleted the fast-cache-extract-deps branch September 22, 2023 16:16
@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 10, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants