Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1808663002:
[Coverage] Add coverage-data-split feature. (Closed)
|
Created:
4 years, 9 months ago by Michael Achenbach Modified:
4 years, 9 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Coverage] Add coverage-data-split feature.
This will allow to only load json data for the files under
review instead of the whole data set.
This will be called on the infra-side after all coverage
data has been merged.
Also fix a bunch of log lines.
BUG=chromium:568949
LOG=n
NOTRY=true
Committed: https://crrev.com/c44b02ba0f567fb4cb89a0a5c91584031c6a323d
Cr-Commit-Position: refs/heads/master@{#34834}
Patch Set 1 #
Total comments: 10
Messages
Total messages: 19 (6 generated)
Description was changed from ========== [Coverage] Add coverage-data-split feature. BUG= ========== to ========== [Coverage] Add coverage-data-split feature. This will allow to only load json data for the files under review instead of the whole data set. This will be called on the infra-side after all coverage data has been merged. BUG=chromium:568949 LOG=n NOTRY=true ==========
Description was changed from ========== [Coverage] Add coverage-data-split feature. This will allow to only load json data for the files under review instead of the whole data set. This will be called on the infra-side after all coverage data has been merged. BUG=chromium:568949 LOG=n NOTRY=true ========== to ========== [Coverage] Add coverage-data-split feature. This will allow to only load json data for the files under review instead of the whole data set. This will be called on the infra-side after all coverage data has been merged. Also fix a bunch of log lines. BUG=chromium:568949 LOG=n NOTRY=true ==========
machenbach@chromium.org changed reviewers: + hablich@chromium.org, kjellander@chromium.org, tandrii@chromium.org
PTAL
https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... File tools/sanitizers/sancov_formatter.py (right): https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:395: new_data = dict(data) I thought you only want the data of the individual file. I am confused. https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:398: new_data['files'] = {file_name: coverage} 'file_name: coverage' does not make sense to me in this context. I assume you mean coverage data, why is it saved in a property called file_name?
Answers https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... File tools/sanitizers/sancov_formatter.py (right): https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:395: new_data = dict(data) On 2016/03/16 13:39:12, Hablich wrote: > I thought you only want the data of the individual file. I am confused. This assignment copies the key/values of data flat (as such is fast). Below I reassign the 'files' key with data of just one file. The other option would have been to explicitly specify the dict again with all keys, which is more code, more error-prone and possibly incomplete (as soon as I add other keys to the format). https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:398: new_data['files'] = {file_name: coverage} On 2016/03/16 13:39:12, Hablich wrote: > 'file_name: coverage' does not make sense to me in this context. I assume you > mean coverage data, why is it saved in a property called file_name? These are not strings. These are the variables from line 384 containing the original key/value pair for one file.
LGTM % assuming I really did miss something. https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... File tools/sanitizers/sancov_formatter.py (right): https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:386: file_path = os.path.join(options.output_dir, file_name + '.json') my only concern here, which I was not able to completely clear myself, is what if file_name contains something like "../../some_dir". The code below actually creates such, which seems not good. What did I miss?
https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... File tools/sanitizers/sancov_formatter.py (right): https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:386: file_path = os.path.join(options.output_dir, file_name + '.json') On 2016/03/16 16:31:31, tandrii(chromium) wrote: > my only concern here, which I was not able to completely clear myself, is what > if file_name contains something like "../../some_dir". The code below actually > creates such, which seems not good. What did I miss? Only normalized relative paths in the project dir are included. We skip the rest in line 131. So all files have the form: 'src/foo/bar.cc' or 'include/baz.h'
lgtm https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... File tools/sanitizers/sancov_formatter.py (right): https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:386: file_path = os.path.join(options.output_dir, file_name + '.json') On 2016/03/16 17:34:00, Michael Achenbach wrote: > On 2016/03/16 16:31:31, tandrii(chromium) wrote: > > my only concern here, which I was not able to completely clear myself, is what > > if file_name contains something like "../../some_dir". The code below actually > > creates such, which seems not good. What did I miss? > > Only normalized relative paths in the project dir are included. We skip the rest > in line 131. So all files have the form: > 'src/foo/bar.cc' or 'include/baz.h' Acknowledged.
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808663002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808663002/1
Message was sent while issue was closed.
Description was changed from ========== [Coverage] Add coverage-data-split feature. This will allow to only load json data for the files under review instead of the whole data set. This will be called on the infra-side after all coverage data has been merged. Also fix a bunch of log lines. BUG=chromium:568949 LOG=n NOTRY=true ========== to ========== [Coverage] Add coverage-data-split feature. This will allow to only load json data for the files under review instead of the whole data set. This will be called on the infra-side after all coverage data has been merged. Also fix a bunch of log lines. BUG=chromium:568949 LOG=n NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [Coverage] Add coverage-data-split feature. This will allow to only load json data for the files under review instead of the whole data set. This will be called on the infra-side after all coverage data has been merged. Also fix a bunch of log lines. BUG=chromium:568949 LOG=n NOTRY=true ========== to ========== [Coverage] Add coverage-data-split feature. This will allow to only load json data for the files under review instead of the whole data set. This will be called on the infra-side after all coverage data has been merged. Also fix a bunch of log lines. BUG=chromium:568949 LOG=n NOTRY=true Committed: https://crrev.com/c44b02ba0f567fb4cb89a0a5c91584031c6a323d Cr-Commit-Position: refs/heads/master@{#34834} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c44b02ba0f567fb4cb89a0a5c91584031c6a323d Cr-Commit-Position: refs/heads/master@{#34834}
Message was sent while issue was closed.
https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... File tools/sanitizers/sancov_formatter.py (right): https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:395: new_data = dict(data) On 2016/03/16 14:13:32, Michael Achenbach wrote: > On 2016/03/16 13:39:12, Hablich wrote: > > I thought you only want the data of the individual file. I am confused. > > This assignment copies the key/values of data flat (as such is fast). Below I > reassign the 'files' key with data of just one file. > > The other option would have been to explicitly specify the dict again with all > keys, which is more code, more error-prone and possibly incomplete (as soon as I > add other keys to the format). Acknowledged. https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:398: new_data['files'] = {file_name: coverage} On 2016/03/16 14:13:32, Michael Achenbach wrote: > On 2016/03/16 13:39:12, Hablich wrote: > > 'file_name: coverage' does not make sense to me in this context. I assume you > > mean coverage data, why is it saved in a property called file_name? > > These are not strings. These are the variables from line 384 containing the > original key/value pair for one file. Huh, I never indicated that I think these are simple strings. It is simply that the naming does not make sense to me at all. Why is the coverage_data saved in a property named file_name? Essentially it is about readability of the code not about its functional side.
Message was sent while issue was closed.
https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... File tools/sanitizers/sancov_formatter.py (right): https://codereview.chromium.org/1808663002/diff/1/tools/sanitizers/sancov_for... tools/sanitizers/sancov_formatter.py:398: new_data['files'] = {file_name: coverage} On 2016/03/17 08:16:47, Hablich wrote: > On 2016/03/16 14:13:32, Michael Achenbach wrote: > > On 2016/03/16 13:39:12, Hablich wrote: > > > 'file_name: coverage' does not make sense to me in this context. I assume > you > > > mean coverage data, why is it saved in a property called file_name? > > > > These are not strings. These are the variables from line 384 containing the > > original key/value pair for one file. > > Huh, I never indicated that I think these are simple strings. It is simply that > the naming does not make sense to me at all. Why is the coverage_data saved in a > property named file_name? > > Essentially it is about readability of the code not about its functional side. How would you map it? The code coverage data contains the covered lines of specific files. The line coverage of each file is collected in a list and mapped to that file's name. I've felt having file names as keys is the most intuitive to get to the coverage data of that specific file. Also in the code review tool that's the data flow. Somebody opens a particular file for review. The file name is the key by which the corresponding coverage data is fetched. In regards to naming code_coverage_data just coverage my opinion is: If the longer name is useful to distinguish code_coverage_data from code_coverage_meta_data or test_coverage_data, etc, I'd go for the long name. But in short methods, where no other concepts exist, I prefer short names.
Message was sent while issue was closed.
nice work. lgtm What about a unit test? Also: are you not worried there will be tons of files stored on GS? Or that's not an issue, not even over time?
Message was sent while issue was closed.
On 2016/03/18 05:04:13, kjellander (chromium) wrote: > nice work. lgtm > > What about a unit test? Hmja, can split the method into the testable part and the part that needs to much mocking/harness. Don't want to bother with fileio in testing. > > Also: are you not worried there will be tons of files stored on GS? Or that's > not an issue, not even over time? It's ~1MB coverage data per patchset compared to a few 100MB going to the isolate server. And I don't wan't to think about how much we're uploading to GS on our CI. E.g. https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20debug%20buil... Also, the many split files are in total roughly the same size compared to the full file. But we're saving a lot of traffic for every view of those files on rietveld as only the file under review will be fetched. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
