Add version of parseJson that can cache file reads and parses #391
Conversation
| /// | ||
| /// Throws away all entries that are not resolvable. | ||
| static Future<Map<String, HitMap>> parseJson( | ||
| static Map<String, HitMap> parseJsonSync( |
There was a problem hiding this comment.
Why add a separate parseJsonSync function? Why not just add a cache to parseJson?
There was a problem hiding this comment.
This - at least I think - would be a non-breaking change.
I think adding it to parseJson (and removing the async) would make it breaking, though I'm quite frankly not sure exactly what constitutes a breaking change.
I've made it sync, btw, to avoid potential race conditions in filling in the cache.
There was a problem hiding this comment.
Yeah, making it sync would be breaking. So if we need a sync version it'll need to be separate.
I wonder if we need the sync version though. I guess the race condition would manifest as occasionally parsing a file twice. Intuitively, I would have thought that's a smaller performance hit than making the function sync.
There was a problem hiding this comment.
I think it's hard to reason about how often a file will be read twice and that's one reason for me to favor the sync version.
As for speed, generally async can be faster when doing multiple io operations simultaneously. If only doing one IO operation at a time though it is slower. parseJson only reads one file at a time. Therefore the only way the async version of parseJson could be faster would be if it was called several times concurrently. Considering parseJson is not optimized for being called several times (at least not if the calls share sources as they for instance do in flutter which is the entire point of the PR) I don't think that's a use-case that should make us favor async over sync.
Said another way, my intuition would certainly be that for the common use case a sync version would be faster than an async version.
| /// | ||
| /// Throws away all entries that are not resolvable. | ||
| static Future<Map<String, HitMap>> parseJson( | ||
| static Map<String, HitMap> parseJsonSync( |
|
CI is redness is unrelated to your PR. I'm working on it. Let's wait for it to turn green before merging your change though. |
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
Oh, just remembered, this will need a minor version bump and a changelog entry
When checkIgnoredLines is true parseJson reads files to find ignore data for that file. If calling parseJson several times for the same project, as for instance done in flutter, this adds significant overhead. This commit adds a new function - parseJsonSync - which is passed a cache of the lookups. This way future lookups for the same file will be faster. The new function is also sync avoiding any potential race conditions when filling up the cache.
jensjoha
force-pushed
the
cacheable_parseJson
branch
from
c0b56e0 to
1c0e6e9
Compare
Done. |
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
LGTM, but let's wait for CI to turn green before merging. I've submitted a fix to the VM, and I'm just waiting for it to roll into dev.
…rchive/coverage#391) * Add version of parseJson that can cache file reads and parses When checkIgnoredLines is true parseJson reads files to find ignore data for that file. If calling parseJson several times for the same project, as for instance done in flutter, this adds significant overhead. This commit adds a new function - parseJsonSync - which is passed a cache of the lookups. This way future lookups for the same file will be faster. The new function is also sync avoiding any potential race conditions when filling up the cache. * Remove createResolver * Feedback: minor version bump and a changelog entry
3 participants
When checkIgnoredLines is true parseJson reads files to find
ignore data for that file.
If calling parseJson several times for the same project,
as for instance done in flutter, this adds significant overhead.
This commit adds a new function - parseJsonSync - which is passed
a cache of the lookups. This way future lookups for the same
file will be faster. The new function is also sync avoiding any
potential race conditions when filling up the cache.
This - in combination with a change on the Flutter side -
reduces the overhead of collecting coverage in Flutter.
On my machine running
flutter test --coverageinflutter/packages/fluttergoes from ~13.5 minutes to ~10 minutes.