Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10443092:
Performance Monitor Database (Closed)
|
Created:
8 years, 6 months ago by eaugusti Modified:
8 years, 5 months ago Reviewers:
Aaron Boodman CC:
Matt Tytel, chebert, clintstaley, aa, Devlin, cduvall, mitchellwrosen Base URL:
http://git.chromium.org/chromium/src.git@cpm_event_construction Visibility:
Public. |
DescriptionPerformance Monitor Database
The database is the core data store for the Performance Monitor.
This CL encapsulates an initial release of the databse without support for metrics or events.
BUG=130212
Patch Set 1 : Synced with changes made in the Event Structure/Construction Cls #
Total comments: 37
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Members for known database instances. #Patch Set 4 : Removed consolidated all the metric databases into one #
Total comments: 13
Patch Set 5 : #Patch Set 6 : scoped_ptr for clock_ #Patch Set 7 : Re-Up #
Messages
Total messages: 15 (0 generated)
Some pre-lunch comments. Not done yet. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... File chrome/browser/performance_monitor/database.cc (right): https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:27: const int kUptimeTimeout = 5 * base::Time::kMicrosecondsPerMinute; Typically we either use base::TimeDelate, or encode the units in the name, so kUptimeTimeoutMinutes. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:44: leveldb::DB* state_db = FetchDBOnBackgroundThread(std::string(kStateDb)); There is implicit conversion from const char[] to std::string, so there's no need for std::string() here. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:63: delete db_iter->second; Use linked_ptr so that you don't need to do the delete manually. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:65: CHECK(file_util::Delete(path_, true)); Document the magic boolean or use a named constant. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:65: CHECK(file_util::Delete(path_, true)); CHECK means: if this condition is false, the code is not behaving as designed, and behavior from here on out is undefined. We may as well crash now. File operations don't fall into that category. File operations can fail for any number of reasons outside the control of the code. The one exception is unit tests. I CHECK file ops in unit tests because it's pointless to carry on if the required files don't exist or whatever, and it indicates a test failure anyway. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... File chrome/browser/performance_monitor/database.h (right): https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:25: class DBClock : public base::RefCountedThreadSafe<DBClock> { These should be nested classes of Database and lose the 'db' prefix. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:25: class DBClock : public base::RefCountedThreadSafe<DBClock> { Why is it necessary for such a simple class to be refcounted? https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:50: typedef std::vector<std::pair<base::Time, base::Time> > TimePairs; std::pair is almost always a mistake. "first" and "second" are meaningless when reading code. I haven't seen how this is used yet, but I already suggest a struct with named fields :) https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:74: FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest, friends are often a smell. Can the functionality the tests rely upon be made generally public? https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:120: LevelDbMap databases_; It appears that there are a fixed set of known leveldb::DB instances. Instead of having a map of these, can they just be explicit members of this class?
https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... File chrome/browser/performance_monitor/database.cc (right): https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:18: const char kDbDir[] = "performance_monitor_dbs"; In Chrome, these files are normally named very human friendly, like: "Performance Monitor Databases" (with caps, spaces, etc) https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:19: const char kRecentDb[] = "_performance_monitor_meta_recent"; Since these are inside the directory, the 'performance_monitor' bit is not necessary, and they can be named like: Recent Events State // though this is so general, I might name it Meta, or Configuration, or something? Activity https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:22: const char kUptimeDb[] = "_performance_monitor_meta_uptime"; 'uptime' has a well understood meaning that is different than how you're using it here, so I'd try to find something else. How about Activity? https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:72: UpdateUptimeOnBackgroundThread(); I think you probably don't want to update here. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:83: if (it->Valid() && it->value().ToString() > start_key) { Scary to compare lexographically. In this case it is fine, but would be nicer to parse and compare numerically. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:99: delete it; scoped_ptr to avoid manual delete? https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:146: leveldb::DB* new_db; = NULL; https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... File chrome/browser/performance_monitor/database.h (right): https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:85: static scoped_refptr<Database> InitOnBackgroundThread( This class seems to have no public way to get an instance. How are people going to call its public instance methods? https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:102: // Mark the database as bring active for the current time. s/bring/being/? https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:120: LevelDbMap databases_; On 2012/06/08 19:33:56, Aaron Boodman wrote: > It appears that there are a fixed set of known leveldb::DB instances. Instead of > having a map of these, can they just be explicit members of this class? Also, I think that it would be good to briefly describe the schema for each of these databases, and why they were chosen the way they were. For example, the uptime database is not immediately intuitive. It would be good to describe the usage so the reader doesn't have to piece it together from the code.
Oh, and one structural comment: consider splitting this class into several - one wrapping each database file. It just seems more natural to have one focused class that knows about the format of each database. And it also gives you a convenient place to describe the layout of that database.
https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... File chrome/browser/performance_monitor/database.cc (right): https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:18: const char kDbDir[] = "performance_monitor_dbs"; On 2012/06/08 21:40:05, Aaron Boodman wrote: > In Chrome, these files are normally named very human friendly, like: > > "Performance Monitor Databases" > > (with caps, spaces, etc) Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:19: const char kRecentDb[] = "_performance_monitor_meta_recent"; On 2012/06/08 21:40:05, Aaron Boodman wrote: > Since these are inside the directory, the 'performance_monitor' bit is not > necessary, and they can be named like: > > Recent > Events > State // though this is so general, I might name it Meta, or Configuration, or > something? > Activity Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:22: const char kUptimeDb[] = "_performance_monitor_meta_uptime"; On 2012/06/08 21:40:05, Aaron Boodman wrote: > 'uptime' has a well understood meaning that is different than how you're using > it here, so I'd try to find something else. > > How about Activity? How about Active Interval? https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:27: const int kUptimeTimeout = 5 * base::Time::kMicrosecondsPerMinute; On 2012/06/08 19:33:56, Aaron Boodman wrote: > Typically we either use base::TimeDelate, or encode the units in the name, so > kUptimeTimeoutMinutes. Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:44: leveldb::DB* state_db = FetchDBOnBackgroundThread(std::string(kStateDb)); On 2012/06/08 19:33:56, Aaron Boodman wrote: > There is implicit conversion from const char[] to std::string, so there's no > need for std::string() here. Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:63: delete db_iter->second; On 2012/06/08 19:33:56, Aaron Boodman wrote: > Use linked_ptr so that you don't need to do the delete manually. Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:65: CHECK(file_util::Delete(path_, true)); On 2012/06/08 19:33:56, Aaron Boodman wrote: > Document the magic boolean or use a named constant. Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:72: UpdateUptimeOnBackgroundThread(); On 2012/06/08 21:40:05, Aaron Boodman wrote: > I think you probably don't want to update here. Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:83: if (it->Valid() && it->value().ToString() > start_key) { On 2012/06/08 21:40:05, Aaron Boodman wrote: > Scary to compare lexographically. In this case it is fine, but would be nicer to > parse and compare numerically. In key generation we pad with enough 0's to make lexicographical comparison safe. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:99: delete it; On 2012/06/08 21:40:05, Aaron Boodman wrote: > scoped_ptr to avoid manual delete? Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.cc:146: leveldb::DB* new_db; On 2012/06/08 21:40:05, Aaron Boodman wrote: > = NULL; Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... File chrome/browser/performance_monitor/database.h (right): https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:25: class DBClock : public base::RefCountedThreadSafe<DBClock> { On 2012/06/08 19:33:56, Aaron Boodman wrote: > Why is it necessary for such a simple class to be refcounted? We had it like this so that tests and the db could share the same clock and the tests could modify the clock as the db was running. But, I guess just using a linked_ptr would probably be a better choice. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:50: typedef std::vector<std::pair<base::Time, base::Time> > TimePairs; On 2012/06/08 19:33:56, Aaron Boodman wrote: > std::pair is almost always a mistake. "first" and "second" are meaningless when > reading code. I haven't seen how this is used yet, but I already suggest a > struct with named fields :) Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:74: FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest, On 2012/06/08 19:33:56, Aaron Boodman wrote: > friends are often a smell. Can the functionality the tests rely upon be made > generally public? We were going to have only the performance monitor main class be able to construct a Database. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:85: static scoped_refptr<Database> InitOnBackgroundThread( On 2012/06/08 21:40:05, Aaron Boodman wrote: > This class seems to have no public way to get an instance. How are people going > to call its public instance methods? Through the main performance monitor class. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:102: // Mark the database as bring active for the current time. On 2012/06/08 21:40:05, Aaron Boodman wrote: > s/bring/being/? Done. https://chromiumcodereview.appspot.com/10443092/diff/9001/chrome/browser/perf... chrome/browser/performance_monitor/database.h:120: LevelDbMap databases_; On 2012/06/08 21:40:05, Aaron Boodman wrote: > On 2012/06/08 19:33:56, Aaron Boodman wrote: > > It appears that there are a fixed set of known leveldb::DB instances. Instead > of > > having a map of these, can they just be explicit members of this class? > > Also, I think that it would be good to briefly describe the schema for each of > these databases, and why they were chosen the way they were. > > For example, the uptime database is not immediately intuitive. It would be good > to describe the usage so the reader doesn't have to piece it together from the > code. Done.
lgtm https://chromiumcodereview.appspot.com/10443092/diff/8012/chrome/browser/perf... File chrome/browser/performance_monitor/database.h (right): https://chromiumcodereview.appspot.com/10443092/diff/8012/chrome/browser/perf... chrome/browser/performance_monitor/database.h:58: // interval. I the end of the active interval was in the key, then every update s/I/If/
https://chromiumcodereview.appspot.com/10443092/diff/8012/chrome/browser/perf... File chrome/browser/performance_monitor/database.h (right): https://chromiumcodereview.appspot.com/10443092/diff/8012/chrome/browser/perf... chrome/browser/performance_monitor/database.h:58: // interval. I the end of the active interval was in the key, then every update On 2012/06/11 21:08:02, Aaron Boodman wrote: > s/I/If/ Done.
http://codereview.chromium.org/10443092/diff/4003/chrome/browser/performance_... File chrome/browser/performance_monitor/database.h (right): http://codereview.chromium.org/10443092/diff/4003/chrome/browser/performance_... chrome/browser/performance_monitor/database.h:10: #include <map> I think this is no longer needed. http://codereview.chromium.org/10443092/diff/4003/chrome/browser/performance_... chrome/browser/performance_monitor/database.h:98: bool AddStateValueOnBackgroundThread(const std::string& key, It looks to me like this class is meant to be used entirely on background threads. In that case, there is no need to label every method 'OnBackgroundThread'. Sorry for leading you down that path ... I only meant it to apply to classes that are used across threads. http://codereview.chromium.org/10443092/diff/4003/chrome/browser/performance_... chrome/browser/performance_monitor/database.h:137: FilePath path, linked_ptr<Clock> clock); Nit: I think it is cleaner to just let the tests use the setter. No need for a constructor that takes a clock. http://codereview.chromium.org/10443092/diff/4003/chrome/browser/performance_... chrome/browser/performance_monitor/database.h:139: static scoped_refptr<Database> InitOnBackgroundThread(FilePath path) { I think this should become a public static factory function, like: static scoped_refptr<Database> Create(const FilePath& path) { ... } There's no need to try and lock down clients to just the ones that we envision at this moment. The class stands alone. Also, I would probably say that the client should always pass a path. Let the normal client pass in the typical path. http://codereview.chromium.org/10443092/diff/4003/chrome/browser/performance_... chrome/browser/performance_monitor/database.h:162: linked_ptr<Clock> clock_; I don't understand why this is not scoped_ptr. http://codereview.chromium.org/10443092/diff/4003/chrome/browser/performance_... chrome/browser/performance_monitor/database.h:164: linked_ptr<leveldb::DB> recent_db_; I don't understand why these databases are linked_ptr. Why not scoped_ptr, or even just normal members? Are you ever going to access these databases from any place other than this class on the background thread?
https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... File chrome/browser/performance_monitor/database.h (right): https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... chrome/browser/performance_monitor/database.h:10: #include <map> On 2012/06/11 22:52:13, Aaron Boodman wrote: > I think this is no longer needed. Done. https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... chrome/browser/performance_monitor/database.h:98: bool AddStateValueOnBackgroundThread(const std::string& key, On 2012/06/11 22:52:13, Aaron Boodman wrote: > It looks to me like this class is meant to be used entirely on background > threads. In that case, there is no need to label every method > 'OnBackgroundThread'. Sorry for leading you down that path ... I only meant it > to apply to classes that are used across threads. Done. https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... chrome/browser/performance_monitor/database.h:137: FilePath path, linked_ptr<Clock> clock); On 2012/06/11 22:52:13, Aaron Boodman wrote: > Nit: I think it is cleaner to just let the tests use the setter. No need for a > constructor that takes a clock. Done. https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... chrome/browser/performance_monitor/database.h:139: static scoped_refptr<Database> InitOnBackgroundThread(FilePath path) { On 2012/06/11 22:52:13, Aaron Boodman wrote: > I think this should become a public static factory function, like: > > static scoped_refptr<Database> Create(const FilePath& path) { > ... > } > > There's no need to try and lock down clients to just the ones that we envision > at this moment. The class stands alone. > > Also, I would probably say that the client should always pass a path. Let the > normal client pass in the typical path. Done. https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... chrome/browser/performance_monitor/database.h:162: linked_ptr<Clock> clock_; On 2012/06/11 22:52:13, Aaron Boodman wrote: > I don't understand why this is not scoped_ptr. When testing, the tests want direct access to the clock at any time. Like if they are populating with some very precise data or need to read the clock. https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... chrome/browser/performance_monitor/database.h:164: linked_ptr<leveldb::DB> recent_db_; On 2012/06/11 22:52:13, Aaron Boodman wrote: > I don't understand why these databases are linked_ptr. Why not scoped_ptr, or > even just normal members? Are you ever going to access these databases from any > place other than this class on the background thread? You are right, they should be linked_ptr.
On 2012/06/12 00:12:57, eaugusti wrote: > https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... > File chrome/browser/performance_monitor/database.h (right): > > https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... > chrome/browser/performance_monitor/database.h:10: #include <map> > On 2012/06/11 22:52:13, Aaron Boodman wrote: > > I think this is no longer needed. > > Done. > > https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... > chrome/browser/performance_monitor/database.h:98: bool > AddStateValueOnBackgroundThread(const std::string& key, > On 2012/06/11 22:52:13, Aaron Boodman wrote: > > It looks to me like this class is meant to be used entirely on background > > threads. In that case, there is no need to label every method > > 'OnBackgroundThread'. Sorry for leading you down that path ... I only meant it > > to apply to classes that are used across threads. > > Done. > > https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... > chrome/browser/performance_monitor/database.h:137: FilePath path, > linked_ptr<Clock> clock); > On 2012/06/11 22:52:13, Aaron Boodman wrote: > > Nit: I think it is cleaner to just let the tests use the setter. No need for a > > constructor that takes a clock. > > Done. > > https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... > chrome/browser/performance_monitor/database.h:139: static > scoped_refptr<Database> InitOnBackgroundThread(FilePath path) { > On 2012/06/11 22:52:13, Aaron Boodman wrote: > > I think this should become a public static factory function, like: > > > > static scoped_refptr<Database> Create(const FilePath& path) { > > ... > > } > > > > There's no need to try and lock down clients to just the ones that we envision > > at this moment. The class stands alone. > > > > Also, I would probably say that the client should always pass a path. Let the > > normal client pass in the typical path. > > Done. > > https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... > chrome/browser/performance_monitor/database.h:162: linked_ptr<Clock> clock_; > On 2012/06/11 22:52:13, Aaron Boodman wrote: > > I don't understand why this is not scoped_ptr. > > When testing, the tests want direct access to the clock at any time. Like if > they are populating with some very precise data or need to read the clock. > > https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... > chrome/browser/performance_monitor/database.h:164: linked_ptr<leveldb::DB> > recent_db_; > On 2012/06/11 22:52:13, Aaron Boodman wrote: > > I don't understand why these databases are linked_ptr. Why not scoped_ptr, or > > even just normal members? Are you ever going to access these databases from > any > > place other than this class on the background thread? > > You are right, they should be linked_ptr. Last comment: s/linked_ptr/scoped_ptr
LGTM w/ one last change. https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... File chrome/browser/performance_monitor/database.h (right): https://chromiumcodereview.appspot.com/10443092/diff/4003/chrome/browser/perf... chrome/browser/performance_monitor/database.h:162: linked_ptr<Clock> clock_; On 2012/06/12 00:12:57, eaugusti wrote: > On 2012/06/11 22:52:13, Aaron Boodman wrote: > > I don't understand why this is not scoped_ptr. > > When testing, the tests want direct access to the clock at any time. Like if > they are populating with some very precise data or need to read the clock. That's fine, you can just have the Database take ownership of the Clock, like this: class Database { ... set_clock(scoped_ptr<Clock> clock) { clock_ = clock.Pass(); } ... scoped_ptr<Clock> clock_; } unittest: scoped_refptr<Database> db = Database::Create(...); TestClock* clock = new TestClock(); db->set_clock(scoped_ptr<Clock>(clock)); clock->DoStuff(42);
FTR, the CQ had issues with @cpm_event_construction in the url. I fixed the CQ.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eaugusti@chromium.org/10443092/12004
Failed to apply patch for chrome/browser/performance_monitor/database.cc: While running patch -p1 --forward --force; The next patch would create the file chrome/browser/performance_monitor/database.cc, which already exists! Skipping patch. 1 out of 1 hunk ignored
The cl has been replaced with this one: file:///home/eriq/bittorent/download/Scott,%20Martin%20-%20Thraxas%20-%20%5Bepub,lit,mobi%5D/nook Which has already been committed.
On 2012/06/18 00:45:34, eaugusti wrote: > The cl has been replaced with this one: > file:///home/eriq/bittorent/download/Scott,%20Martin%20-%20Thraxas%20-%20%5Bepub,lit,mobi%5D/nook > > Which has already been committed. Oops, didn't override buffer: https://chromiumcodereview.appspot.com/10545139/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
