Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2447443002:
Log the size of each of the stores and complete DB on launch and after each (Closed)
|
Created:
4 years, 2 months ago by vakh (use Gerrit instead) Modified:
4 years, 1 month ago CC:
chromium-reviews, grt+watch_chromium.org, vakh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog the size of each of the stores and complete DB on launch and after each
update.
See: https://screenshot.googleplex.com/HFnJ5zLQwwo
TBR=holte
BUG=543161
Committed: https://crrev.com/8ca75275c18c22b084ac44fdfd4247f983b09809
Cr-Commit-Position: refs/heads/master@{#428093}
Patch Set 1 #Patch Set 2 : Add to histograms.xml #
Total comments: 12
Patch Set 3 : shess@'s review #
Total comments: 7
Patch Set 4 : Use int64_t consistently. Undo changes outside v4 #Patch Set 5 : git rebase #
Total comments: 4
Patch Set 6 : nparker@'s review #Patch Set 7 : Nit: clarify comment #Patch Set 8 : git rebase #
Messages
Total messages: 59 (33 generated)
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Add to histograms.xml
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Log the size of each of the stores and complete DB on launch and after each update. BUG=543161 ========== to ========== Log the size of each of the stores and complete DB on launch and after each update. See: https://screenshot.googleplex.com/HFnJ5zLQwwo BUG=543161 ==========
vakh@chromium.org changed reviewers: + nparker@chromium.org, shess@chromium.org
A meta-concern is that since this will be getting updated periodically, and there are users who leave Chrome running continuously, so there is kind of a built-in bias towards long-running users. I never was able to generate a cogent argument for how to change things, but maybe this is your opportunity to think of something. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_database.cc:225: int db_size = 0; These ints should all be int64_t to match FileSizeOrZero(). https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.cc (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.cc:516: int64_t size_64; Maybe strip the _64 while you're at it. I don't recall why it was in there in the first place, but hardly necessary for a 4-line function. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:703: file_size_ = V4ProtocolManagerUtil::GetFileSizeOrZero(store_path_); Could you get this from contents.size()? https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:739: file_size_ = V4ProtocolManagerUtil::GetFileSizeOrZero(store_path_); Could this ever be a different value from |written|? [If not ... then my other comment about file_size_ being an int64_t switches to being a size_t.] https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.h:175: int RecordAndReturnFileSize(const std::string& base_metric); This should be int64_t. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.h:388: int file_size_; This should be int64_t.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
shess@'s review
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/21 23:36:45, Scott Hess wrote: > A meta-concern is that since this will be getting updated periodically, and > there are users who leave Chrome running continuously, so there is kind of a > built-in bias towards long-running users. I never was able to generate a cogent > argument for how to change things, but maybe this is your opportunity to think > of something. > The size of the file shouldn't really differ between users who keep Chrome running for a long time and those who don't. Unless, those who don't run it for such a short period of time that they don't get any SafeBrowsing update, i.e. less than 18 minutes, which may be unusual. 18 mins because that's the mean of the min (5) and max (30) wait minutes before fetching an update.
https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_database.cc:225: int db_size = 0; On 2016/10/21 23:36:45, Scott Hess wrote: > These ints should all be int64_t to match FileSizeOrZero(). Done. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.cc (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.cc:516: int64_t size_64; On 2016/10/21 23:36:45, Scott Hess wrote: > Maybe strip the _64 while you're at it. I don't recall why it was in there in > the first place, but hardly necessary for a 4-line function. Done. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:703: file_size_ = V4ProtocolManagerUtil::GetFileSizeOrZero(store_path_); On 2016/10/21 23:36:45, Scott Hess wrote: > Could you get this from contents.size()? Done. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:739: file_size_ = V4ProtocolManagerUtil::GetFileSizeOrZero(store_path_); On 2016/10/21 23:36:45, Scott Hess wrote: > Could this ever be a different value from |written|? > > [If not ... then my other comment about file_size_ being an int64_t switches to > being a size_t.] Done. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.h:175: int RecordAndReturnFileSize(const std::string& base_metric); On 2016/10/21 23:36:45, Scott Hess wrote: > This should be int64_t. Done. https://codereview.chromium.org/2447443002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.h:388: int file_size_; On 2016/10/21 23:36:45, Scott Hess wrote: > This should be int64_t. Done.
Hmm - now the member storage is uint64_t, but some code uses int64_t, and other code gets the value from a size_t. I am not at all certain why base::GetFileSize() uses int64_t, and I'm not sure it would improve my quality of life to figure it out. I am positive that size_t is the thing to use for "Things in memory that can be counted." FileSizeOrZero() explicitly grants latitude for conversion, so I think my most-happy-minimal-casting version is size_t. But if not size_t, then the problem is that I think most style has it as int (or int64_t) for generic counting of things. My argument for int64_t in that case is that GetFileSize() uses int64_t, probably because files could be larger than a 32-bit integer. Until such time as we get the go-ahead to assume sizeof(int)>=8 and GetFileSize() stops being explicit, the code using the results of GetFileSize() should be explicit. Which I think means that int64_t is the right choice (since size_t can only be guaranteed sufficient for a single store in isolation). Which does imply static_cast<int64_t>(contents.size()). [Sorry to be so bothersome. If one bit of the code is pedantic about int size, the client code should either be likewise pedantic, or explicitly cast away the pedantic bit.]
On 2016/10/24 20:00:03, Scott Hess wrote: > [Sorry to be so bothersome. If one bit of the code is pedantic about int size, > the client code should either be likewise pedantic, or explicitly cast away the > pedantic bit.] To be clear, I'm fine with either approach. If GetFileSizeOrZero() returned an int and was documented to be wrong if your file size can outrun an int, then everything else could be int and that's fine. It's just the mixture which concerns me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.h (right): https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.h:270: static int64_t GetFileSizeOrZero(const base::FilePath& file_path); Are you using this in v4? If not, why move it? https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:851: base_metric + suffix, 1, 1000000, 50, nit: I think this would be better as '1, 10000, 50' As is this will generate the following buckets (I think): % python >>> import numpy as np >>> [int(x*10)/10. for x in np.logspace(0, 5, 50)] [1.0, 1.2, 1.5, 2.0, 2.5, 3.2, 4.0, 5.1, 6.5, 8.2, 10.4, 13.2, 16.7, 21.2, 26.8, 33.9, 42.9, 54.2, 68.6, 86.8, 109.8, 138.9, 175.7, 222.2, 281.1, 355.6, 449.8, 568.9, 719.6, 910.2, 1151.3, 1456.3, 1842.0, 2329.9, 2947.0, 3727.5, 4714.8, 5963.6, 7543.1, 9540.9, 12067.9, 15264.1, 19306.9, 24420.5, 30888.4, 39069.3, 49417.1, 62505.5, 79060.4, 100000.0] That starts to lose resolution in the relevant range of ~1MB (1151KB -> 1456 KB). If you set the max to 100MB we'll get better resolution there: >>> [int(x*10)/10. for x in np.logspace(0, 4, 50)] [1.0, 1.2, 1.4, 1.7, 2.1, 2.5, 3.0, 3.7, 4.4, 5.4, 6.5, 7.9, 9.5, 11.5, 13.8, 16.7, 20.2, 24.4, 29.4, 35.5, 42.9, 51.7, 62.5, 75.4, 91.0, 109.8, 132.5, 159.9, 193.0, 232.9, 281.1, 339.3, 409.4, 494.1, 596.3, 719.6, 868.5, 1048.1, 1264.8, 1526.4, 1842.0, 2222.9, 2682.6, 3237.4, 3906.9, 4714.8, 5689.8, 6866.4, 8286.4, 10000.0] https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.h:166: const uint64_t old_file_size = 0); I'm not clear on why you need the old_file_size. Every store will have a size either from reading from disk, or writing to disk. Ya?
https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.h:166: const uint64_t old_file_size = 0); On 2016/10/24 20:33:55, Nathan Parker wrote: > I'm not clear on why you need the old_file_size. Every store will have a size > either from reading from disk, or writing to disk. Ya? Yes, except what do we want to report if the update succeeds in memory, but writing the file to disk fails. When we get an update, we write it to disk and if that happens successfully, we set file_size_ = written. Now imagine the case of being unable to write the new file to disk. We could set file_size_ to '0' in this case and be done with it. However, IMO, that would be wrong. What we are interested in is the size of the store on disk. If the new file fails to be written, the old file is still there. So, we can inherit the size of the old file from the old_store and update it if the write to disk occurs successfully. That's why we need this parameter.
Use int64_t consistently. Undo changes outside v4
https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.h (right): https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.h:270: static int64_t GetFileSizeOrZero(const base::FilePath& file_path); On 2016/10/24 20:33:54, Nathan Parker wrote: > Are you using this in v4? If not, why move it? I was. Then shess@ pointed out that I don't need to. Reverted. https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:851: base_metric + suffix, 1, 1000000, 50, On 2016/10/24 20:33:54, Nathan Parker wrote: > nit: I think this would be better as '1, 10000, 50' > > As is this will generate the following buckets (I think): > > % python > >>> import numpy as np > >>> [int(x*10)/10. for x in np.logspace(0, 5, 50)] > [1.0, 1.2, 1.5, 2.0, 2.5, 3.2, 4.0, 5.1, 6.5, 8.2, 10.4, 13.2, 16.7, 21.2, 26.8, > 33.9, 42.9, 54.2, 68.6, 86.8, 109.8, 138.9, 175.7, 222.2, 281.1, 355.6, 449.8, > 568.9, 719.6, 910.2, 1151.3, 1456.3, 1842.0, 2329.9, 2947.0, 3727.5, 4714.8, > 5963.6, 7543.1, 9540.9, 12067.9, 15264.1, 19306.9, 24420.5, 30888.4, 39069.3, > 49417.1, 62505.5, 79060.4, 100000.0] > > That starts to lose resolution in the relevant range of ~1MB (1151KB -> 1456 > KB). If you set the max to 100MB we'll get better resolution there: > > >>> [int(x*10)/10. for x in np.logspace(0, 4, 50)] > [1.0, 1.2, 1.4, 1.7, 2.1, 2.5, 3.0, 3.7, 4.4, 5.4, 6.5, 7.9, 9.5, 11.5, 13.8, > 16.7, 20.2, 24.4, 29.4, 35.5, 42.9, 51.7, 62.5, 75.4, 91.0, 109.8, 132.5, 159.9, > 193.0, 232.9, 281.1, 339.3, 409.4, 494.1, 596.3, 719.6, 868.5, 1048.1, 1264.8, > 1526.4, 1842.0, 2222.9, 2682.6, 3237.4, 3906.9, 4714.8, 5689.8, 6866.4, 8286.4, > 10000.0] > Unless I am looking at it incorrectly that's 10 MB. Since the size of Soceng list is ~5MB already, I think 10 MB may be too low. How about 50,000? >>> [int(x*10)/10. for x in np.logspace(0, 4.699, 50)] [1.0, 1.2, 1.5, 1.9, 2.4, 3.0, 3.7, 4.6, 5.8, 7.2, 9.0, 11.3, 14.1, 17.6, 22.0, 27.4, 34.2, 42.6, 53.2, 66.3, 82.7, 103.2, 128.7, 160.5, 200.2, 249.7, 311.4, 388.3, 484.3, 604.0, 753.2, 939.3, 1171.4, 1460.9, 1821.9, 2272.1, 2833.5, 3533.6, 4406.7, 5495.6, 6853.5, 8547.0, 10658.9, 13292.6, 16577.1, 20673.2, 25781.3, 32151.7, 40096.0, 50003.4]
https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:851: base_metric + suffix, 1, 1000000, 50, On 2016/10/24 22:04:51, vakh (Varun Khaneja) wrote: > Unless I am looking at it incorrectly that's 10 MB. > Since the size of Soceng list is ~5MB already, I think 10 MB may be too low. How > about 50,000? Flipping the question over, do you really care about files under 100k? 50k? Noticing that the files are growing seems more actionable than noticing that they are getting exceedingly small.
On 2016/10/24 22:11:38, Scott Hess wrote: > https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... > File components/safe_browsing_db/v4_store.cc (right): > > https://codereview.chromium.org/2447443002/diff/40001/components/safe_browsin... > components/safe_browsing_db/v4_store.cc:851: base_metric + suffix, 1, 1000000, > 50, > On 2016/10/24 22:04:51, vakh (Varun Khaneja) wrote: > > Unless I am looking at it incorrectly that's 10 MB. > > Since the size of Soceng list is ~5MB already, I think 10 MB may be too low. > How > > about 50,000? > > Flipping the question over, do you really care about files under 100k? 50k? > Noticing that the files are growing seems more actionable than noticing that > they are getting exceedingly small. Some lists (whitelists) will be small. Knowing somewhat accurately how big they are should be useful since being on a whitelist means getting a free pass.
git rebase
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vakh@chromium.org changed reviewers: + holte@chromium.org
holte@ -- please review the changes in histograms.xml
lgtm https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store.h:163: // 0 otherwise. Add something like "old_file_size is logged to UMA even even if the write to disk fails," per your explanation. Or this could go in the code around where we log it to uma. https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:138: EXPECT_EQ(14l, store.file_size_); Is there a case where it should eq 0, like on a failed read from disk?
https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store.h:163: // 0 otherwise. On 2016/10/25 19:01:44, Nathan Parker wrote: > Add something like "old_file_size is logged to UMA even even if the write to > disk fails," per your explanation. Or this could go in the code around where we > log it to uma. Done. https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2447443002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:138: EXPECT_EQ(14l, store.file_size_); On 2016/10/25 19:01:44, Nathan Parker wrote: > Is there a case where it should eq 0, like on a failed read from disk? Done.
nparker@'s review
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nit: clarify comment
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
holte@ -- please review the small change in histograms.xml (one histogram added). Thanks.
Description was changed from ========== Log the size of each of the stores and complete DB on launch and after each update. See: https://screenshot.googleplex.com/HFnJ5zLQwwo BUG=543161 ========== to ========== Log the size of each of the stores and complete DB on launch and after each update. See: https://screenshot.googleplex.com/HFnJ5zLQwwo TBR=holte BUG=543161 ==========
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2447443002/#ps120001 (title: "Nit: clarify comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
git rebase
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2447443002/#ps140001 (title: "git rebase")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Log the size of each of the stores and complete DB on launch and after each update. See: https://screenshot.googleplex.com/HFnJ5zLQwwo TBR=holte BUG=543161 ========== to ========== Log the size of each of the stores and complete DB on launch and after each update. See: https://screenshot.googleplex.com/HFnJ5zLQwwo TBR=holte BUG=543161 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Log the size of each of the stores and complete DB on launch and after each update. See: https://screenshot.googleplex.com/HFnJ5zLQwwo TBR=holte BUG=543161 ========== to ========== Log the size of each of the stores and complete DB on launch and after each update. See: https://screenshot.googleplex.com/HFnJ5zLQwwo TBR=holte BUG=543161 Committed: https://crrev.com/8ca75275c18c22b084ac44fdfd4247f983b09809 Cr-Commit-Position: refs/heads/master@{#428093} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8ca75275c18c22b084ac44fdfd4247f983b09809 Cr-Commit-Position: refs/heads/master@{#428093} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
