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

Discussion/archive: use custom dedup table #302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bbrehm
Copy link
Contributor

@bbrehm bbrehm commented Jan 27, 2025

This replaces scala.collection.mutable.LinkedHashMap[String, Int] with a custom dedup table for construction of the string-pool. That brings down serialization on my large test graph from ~2200ms to ~1700ms, i.e. does a hefty chunk.

Main reason why this does so much is that all the string deduplication must be done serially, in order to allow us to get bitwise-reproducible results; and now that everything else is parallel, this means that this becomes a bottleneck if there are enough cores.

I'm pretty unsure whether the speedup is worth the maintenance hassle. If we don't want to merge, then it is fine to leave this PR open indefinitely, in case we want to resurrect it in the future.

@bbrehm bbrehm requested review from mpollmeier and ml86 January 27, 2025 13:13
Copy link
Contributor

@mpollmeier mpollmeier left a comment

Choose a reason for hiding this comment

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

I'm in favor of bringing this in, but first please:

  1. add a unit test

  2. copy the good explanation from the PR description into the code, so it's accessible in the future, and in case we decide it becomes a maintance burden (which I doubt) we can safely revert it / kick it out

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