1381972 - Speed up lcov rewriter

Status: RESOLVED FIXED

(Testing :: Code Coverage, enhancement)

Description

[Marco Castelluccio [:marco]]

It is taking really long to run the rewriter (~2 hours and 15 minutes on a c4.large Amazon instance).

Could we make it faster?

- For the paths, we can generate a JSON file and let grcov use it to rewrite them;
- For the merging of different entries with the same SF value, we can rely on grcov (it is already doing it);
- The preprocessor rewriting is the only thing left.

Comment 1

[Kyle Lahnakoski [:ekyle]]

Spot pricing for c4.large is about $0.025/hour, we run about 100 jobs for coverage, about 10x per day, for a cost of $25/day.  The main slowdown is probably the EBS drives. Can we speed it up by using a different instance with a local ephemeral drive?

Comment 2

[Marco Castelluccio [:marco]]

The rewriting is only performed once in the uploader task, not for every test task (as it requires the source code to be present), so it should be around $0.5 per day.

Comment 3

[Marco Castelluccio [:marco]]

I think we could:
1) Only parse and rewrite records associated with preprocessed files, for other records we will only parse the source names;
2) Reduce the number of preprocessed files, so we reduce the number of records we need to fully parse (bug 1384044 is one of the main offenders, but it's a small file, so cheaper to rewrite);
3) Accept zip files as input, so users of lcov_rewriter.py don't need to unzip files first;

Regarding 1, for records we don't fully parse, we could directly rewrite the SF entry only and output the rest of the record as-is (so without parsing the full record and calling LcovFile.format_record).

Comment 4

[Marco Castelluccio [:marco]]

What do you think of comment 3? Other ideas?

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment 3 sounds reasonable and will probably speed things up a lot. I know that reducing the number of preprocessed js files has been a goal in general.

Other than that, I suppose should be able to do these in parallel, but I expect the other optimizations you mention would be enough.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Part 1: Only fully parse records belonging to preprocessed files, as we only need to rewrite those

This is a WIP for part 1.

There's still a test failure that I need to figure out in test_remap_lcov:
> self.assertEqual(original_line_count,sum(r.line_count for r in records))
fails because original_line_count is 1083, the sum of the records' line counts is 2133.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Part 1: Only fully parse records belonging to preprocessed files, as we only need to rewrite those

Test fixed.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Part 2: Make lcov_rewriter.py use LCOV files from a ZIP file directly

Interestingly, using lcov_rewriter.py with a ZIP file is 2.5x slower than unzipping first (still in Python using the same ZipFile library) and giving lcov_rewriter.py a list of files.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8891269 [details] [diff] [review]
Part 1: Only fully parse records belonging to preprocessed files, as we only need to rewrite those

Review of attachment 8891269 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. A question and a nit, please address those before landing.

::: python/mozbuild/mozbuild/codecoverage/lcov_rewriter.py
@@ +260,2 @@
>              if line == 'end_of_record':
> +                if current_source_file != '':

Are we ever going to see 'end_of_record' without a 'current_source_file'?

@@ +694,3 @@
>  
> +        with open(in_path) as fh:
> +            with open(out_path, 'w+') as out_fh:

These can be collapsed to one line: "with open(in_path) as in_fh, open(out_path) as out_fh:"...

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Chris Manchester (:chmanchester) from comment #9)
> Comment on attachment 8891269 [details] [diff] [review]
> Part 1: Only fully parse records belonging to preprocessed files, as we only
> need to rewrite those
> 
> Review of attachment 8891269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. A question and a nit, please address those before landing.
> 
> ::: python/mozbuild/mozbuild/codecoverage/lcov_rewriter.py
> @@ +260,2 @@
> >              if line == 'end_of_record':
> > +                if current_source_file != '':
> 
> Are we ever going to see 'end_of_record' without a 'current_source_file'?

If we are unable to rewrite the URL (that is if `rewrite_url` returns None), then current_source_file is going to be ''.
It's the same behavior as before the patch, as we were adding the records whose URL we couldn't rewrite to the `removals` set and then we were ignoring those records.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9d3b4adf01
Only fully parse records belonging to preprocessed files, as we only need to rewrite those. r=chmanchester

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2b9d3b4adf01