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

performance: parallelize diffgraph application #288

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bbrehm
Copy link
Contributor

@bbrehm bbrehm commented Jan 9, 2025

Can you @mpollmeier test that on the same thing you use to evaluate ArrayList vs ArrayBuffer?
Can you test this on a big cpg creation?

(the issue with cpg creation is temp memory during diffgraph application. If we parallelize, then more stuff is alive at the same time. I also had to remove some early clearing of memory, because that would require more synchronization to figure out when an object is truly dead)

@mpollmeier
Copy link
Contributor

thank you, will do 👍🏻

@bbrehm
Copy link
Contributor Author

bbrehm commented Jan 9, 2025

Ok, it wasn't so bad to restore the early cleanup logic (so that the GC can reclaim structures that are not needed anymore, even while the diffgraph application is still running).

Main things to consider are:

  1. The atomic refcount rigamarole
  2. The Callable closure is kept alive by the java.util.concurrent.ForkJoinTask.AdaptedCallable after it finished, until we do submissions.clear(). So we need to be careful about what we capture in the closure!

@mpollmeier
Copy link
Contributor

import timing results: #286 (comment)

@mpollmeier mpollmeier self-requested a review January 10, 2025 12:36
@mpollmeier
Copy link
Contributor

Won't merge for now, because the performance gains currently don't justify the added complexity. But this might come in handy in future, so let's keep the PR open.

Context:
#286

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.

2 participants