-
Notifications
You must be signed in to change notification settings - Fork 150
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
MergeFiles improvements #1309
Comments
Hello @adamdecaf, Just got to know about moov and its OSS community. I must say this is great and beautiful. I am golang engineer and recently looking into perfomance engineering. I will love to help take a look at issue and investigate and give a report on how it can be carried out if given the opportunity to. Looking forward to hearing from you. Thank you |
Sounds good! Let me know if you need anything or have questions. Do you have a place to start? |
Yes, I do. Will try to familiarize myself with the codebase and investigate properly. If i do have questions, i will relate them to you. Thank you! |
I have some questions regarding this statement. Do you mean I should verify which is more efficient between merging files directly from the disk as they are being read and merging the files after they have been read and converted to |
There are a few options available. We currently need the files in memory (as an There may be faster ways to merge I've linked a few PR's in the description which add benchmarks or previous performance improvements. |
Yea! Thanks for that |
Hello @adamdecaf Not sure I was able to pull this off. But i will love to learn more about this codebase in general and probably pick up smaller tasks so i can get used to it because I could not really understand some parts of the code itself. The only recommedation i have got is why we are using []*file and not []file because the latter has more memory perks than the former. Thank you. If there are any smaller tasks i could take on that could get me used to the codebase, I will be really happy to pick it up. |
Why would |
I think that it is a general rule of thumb to try as much as possible to reduce the use of pointers in a program because it thereby reduces the amount of heap allocations done. It is not always so actually but it is so common in golang programs -- compiler generates a lot of checks when using pointer which makes programs a bit slower Just some opinions regarding that |
We'd need to compare benchmarks between MergeFiles with and without pointers. Typically pointers to larger structs are more efficient and faster. See https://medium.com/@meeusdylan/when-to-use-pointers-in-go-44c15fe04eac |
Yes Hm, Looking at it now, trying generate becnchmark reports for both cases. Quite a deep change in to the codebase. Not sure if we are ready to risk that at this point or i can go ahead. |
You can make whatever changes on a branch to compare pointers vs no pointers. |
Oh okay perfect |
By the way just to note here that it has this very approach proven that this is will improve the memory performance but reduce the cpu time significantly which i believe is not what we desire. here some resources I found regarding that
|
We have a big usecase in |
Lots of fixes were released in https://github.com/moov-io/ach/releases/tag/v1.37.0 |
The Moov ACH library has a few ways of merging ACH files together, which can be useful to minimize the number of files sent over the network and cost from vendors, Fed, or other providers.
The performance and utility of MergeFiles can be improved and this issue sets out to discuss a few options.
MergeFiles(files []*ach.File) ([]*ach.File, error)
*ach.File
instances, or another input?)Previous issues: #1041, #1276, #1282
The text was updated successfully, but these errors were encountered: