Coverage Converter update #25
Conversation
| } | ||
|
|
||
| // Subrange information struct | ||
| struct Subrange: Codable { |
There was a problem hiding this comment.
Since all we're doing here is decoding JSON, you could change this (and the ones below) to use Decodable instead of Codable.
| struct Subrange: Codable { | |
| struct Subrange: Decodable { |
| // Custom encoding method to handle the top-level dictionary | ||
| func encode(to encoder: Encoder) throws { | ||
| var container = encoder.container(keyedBy: CodingKeys.self) | ||
| for (key, value) in files { | ||
| guard let codingKey = CodingKeys(stringValue: key) else { | ||
| continue | ||
| } | ||
| try container.encode(value, forKey: codingKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
If you switch to only using Decodable, you can then remove this custom encoding method.
| // Custom encoding method to handle the top-level dictionary | |
| func encode(to encoder: Encoder) throws { | |
| var container = encoder.container(keyedBy: CodingKeys.self) | |
| for (key, value) in files { | |
| guard let codingKey = CodingKeys(stringValue: key) else { | |
| continue | |
| } | |
| try container.encode(value, forKey: codingKey) | |
| } | |
| } |
| let lineNum = lineData.line | ||
| var covered = lineData.executionCount | ||
| if covered != nil { |
There was a problem hiding this comment.
Since we're not doing anything if covered == nil, could we switch this to a guard statement? It'd eliminate the need for force unwrapping covered below as well.
| let lineNum = lineData.line | |
| var covered = lineData.executionCount | |
| if covered != nil { | |
| let lineNum = lineData.line | |
| guard var covered = lineData.executionCount else { | |
| ... |
|
That looks very good to me as well. A change I should have done in the first place. Please give me one more day to review. I am pretty loaded with work "at work" at the moment. Will come back to you on this tomorrow |
|
Hi @maxwell-legrand could you fix the tests on your branch, so the action runs green? I can not commit the fixed tests to your repo. |
| arguments.append("--json") | ||
| arguments.append(resultFile.url.path) | ||
| let coverageData = try Shell.execute(program: "/usr/bin/xcrun", with: arguments) | ||
| let resultsString = String(decoding: coverageData, as: UTF8.self) |
There was a problem hiding this comment.
No need to convert coverageData to String and then back to Data.
Instead we can use
guard let coverageJson = try? JSONDecoder().decode(FileCoverage.self, from: coverageData) else
...
right away. Right?
Skip line 122 to 127
| let fileInfoSemaphore = DispatchSemaphore(value: 1) | ||
| var fileInfo: [FileInfo] = [] | ||
|
|
||
| // since we need to invoke xccov for each file, it takes pretty much time | ||
| // so we invoke it in parallel on 8 threads, that speeds up things considerably | ||
| let queue = OperationQueue() |
There was a problem hiding this comment.
Now that xcccv tool doesn't need to get called repeatedly we do not need to spread the for loop over different threads. In fact I testest and without spawning to threads it ran slightly faster.
There was a problem hiding this comment.
Hey there, sorry for the delay here, haven't been at my computer much this weekend. Is there a way to toggle this to be a CLI flag? I imagine that for a very large results file it might be faster to work serially on a machine with not a lot of CPUs but on a machine with 8+ cpus I image it might eek out better performance? I can try and work on a PR to add that functionality if you'd be alright with that
There was a problem hiding this comment.
I was just saving the CPU for other parallel tasks. I will try some larger xcresult bundles and measure the difference with the PR I just created and with your version. Let's see, it it is worth it to block more than one core for this. And let's take this discussion to the new PR. I invited you as collaborator and once you accept I can add you as reviewer to the PR.
There was a problem hiding this comment.
Sounds good to me, I'm not all too familiar with Swift's parallelism and my best frame of reference is Go's goroutines (greenthreads)
This change was brought about by a necessity to optimize how the coverage for each file was generated. On my machine, running the tool for a large xcresult file would take a long time to complete (roughly 2-2.5 minutes) and would cause a significant draw of CPU as multiple instances of xccov were being run.
I've attempted to improve the performance by instead making one call to xccov and generating a JSON report for the whole xcresult file and then iterate over the keys in parallel. This brings the time for running the tool against the same file from 2 minutes to 4 seconds.
This is my first attempt at writing real Swift, so feel free to tear this apart with feedback! Appreciate all the amazing work that has gone into this project! Thanks in advance!