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

feat: add StringWithConcurrency function #202

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

mdross95
Copy link
Contributor

@mdross95 mdross95 commented Jun 20, 2024

Proposal for adding StringWithConcurrency function to file_instance.

When testing metro2 file generation with a large amount of segments with moov metro2, I noticed that the String() function in file_instance can take a decent amount of time (i.e. ~15 minutes on my machine for close to ~200k segments).

To alleviate this, I implemented a simple StringWithConcurrency function in our fork that takes in a desired # of concurrent goroutines, splits up the base segment processing into that amount of chunks, processes them in parallel, and merges them at the end.

In my testing, leveraging this function has allowed us to reduce the String() generation time by over tenfold.

We are now using this function in our fork for furnishment process - I figured I would open a PR if moov thinks it is valuable to the main repo. Please let me know what you think - thanks!

Copy link
Member

@adamdecaf adamdecaf left a comment

Choose a reason for hiding this comment

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

This makes sense. Thanks!

@adamdecaf adamdecaf merged commit 7463b3e into moov-io:master Jun 20, 2024
6 checks passed
@adamdecaf
Copy link
Member

I'm thinking that ConcurrentString(isNewline, goroutines) sounds better. Thoughts?

@adamdecaf
Copy link
Member

Also, is there a reason we don't default this as runtime.NumCPU() ?

@mdross95
Copy link
Contributor Author

ConcurrentString works for me, and sure, we could default it to that in String() instead of sending 1 👍

@adamdecaf
Copy link
Member

eb9fc66

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