Improve performance of collate & reduce memorty consumption#960
Conversation
|
|
||
| # Use the ResultMerger to produce a single, merged result, ready to use. | ||
| @result = ResultMerger.merge_and_store(*results) | ||
| # TODO: Did/does collate ignore old results? It probably shouldn't, right? |
There was a problem hiding this comment.
pretty sure it didn't/doesn't in the past but does right now which means we'll likely have to introduce a boolean flag to merge_and_store.
| # to have "old" resultsets lying around (but not too old so that they're still | ||
| # considered we can adapt them). | ||
| # See https://github.com/simplecov-ruby/simplecov/pull/824#issuecomment-576049747 | ||
| def adapt_result(result) |
There was a problem hiding this comment.
code had to be moved since we don't create Result objects for merging any more
| end | ||
| end | ||
|
|
||
| # This is dropping data of files that can not be found... why though? |
There was a problem hiding this comment.
seriously... why? Can't we just let it in there and it'll be dropped on result generation anyhow as we can't display it? Probaby have to do some git digging on this to find the reason.
There was a problem hiding this comment.
It has existed basically forever: 4f37de8
| # SimpleCov::Result with merged coverage data and the command_name | ||
| # for the result consisting of a join on all source result's names | ||
| # | ||
| # TODO: Maybe put synchronization just around the reading? |
There was a problem hiding this comment.
yeah so right now it synchronizes including the whole merge time which might be a slow down - probably means extracting onr or 2 smaller methods and just puttign them together but should be worth it
|
|
||
| # Clear out the previously cached .resultset | ||
| def clear_resultset | ||
| @resultset = nil |
There was a problem hiding this comment.
resultset was only ever called in library code right after clearing it, so the whole clear and cache mechanism wasn't much good (unless I missed something)
| @@ -0,0 +1,3 @@ | |||
| # some comment | |||
| puts "wargh" | |||
| puts "wargh 1" | |||
There was a problem hiding this comment.
these files were just missing but referenced in tests and sicne we have code that removes missing files from saved results it caused problems so added them :)
|
@PragTob I'm sorry I can't find time right now to review this 😞 |
|
@deivid-rodriguez no worries at all! 💚 Just thought it'd be great. I'll merge it this afternoon, if at some point later you find the time I'd be happy but no pressure or worries at all. Happy new year and I hope that all is well! 🚀
|
We used to read all files into memory - which gets too much if someone runs a massively parallel CI because 400 * 10MB is still ~4GB and that's just pure file size and we do a lot more with it. Breaks the interface of ResultMerger.merge_and_store but it's not intended as a public interface. Will leave a note in the Changelog anyhow. What is attempted to do top leve is perhaps easier to see when looking at the spike code: https://github.com/simplecov-ruby/simplecov/compare/collate-plus-plus?expand=1 Changes go further than just not reading all files in at once, during the merge process we also operate on the raw file structure as opposed to creating SimpleCov::Result. Creating SimpleCov::Result comes with a lot of overhead, notably reading in all source files. So, that's even worse doing ~400 times in a large code base. There's more optimization potential for cases like these which I'll open a ticket about but notably: * Potentially don't create SimpleCov::Result at all until we really produce results (just dump the raw coverage more or less) * allow running without a formatter as only the last one really needs the formatter
* lets instead of instance variables * less nesting but hopefully more meaningful
df37201 to
a3247f7
Compare
2 participants
Draft to get early feedback! (cc: @deivid-rodriguez ? :D ) So Todos and fixmes are intended and will either get resolved or turned into tickets.
I'm worried about breaking things, as the work to move my quick performance hack to main line code was more involved than I had hoped.
I haven't measured this again but memory consumption and speed gains were massive with the initial spike. Perf was ~2x for my smaller sample set and it allowed the big sample set to do its thing at all (I killed it at 12GB memory consumption).
We used to read all files into memory - which gets too much if
someone runs a massively parallel CI because 400 * 10MB is still
~4GB and that's just pure file size and we do a lot more with it.
Breaks the interface of ResultMerger.merge_and_store but it's not
intended as a public interface. Will leave a note in the Changelog
anyhow.
What is attempted to do top leve is perhaps easier to see when
looking at the spike code: https://github.com/simplecov-ruby/simplecov/compare/collate-plus-plus?expand=1
Changes go further than just not reading all files in at once,
during the merge process we also operate on the raw file structure
as opposed to creating SimpleCov::Result. Creating SimpleCov::Result
comes with a lot of overhead, notably reading in all source files. So,
that's even worse doing ~400 times in a large code base.
There's more optimization potential for cases like these which
I'll open a ticket about but notably:
produce results (just dump the raw coverage more or less)
needs the formatter