Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829342:
Add BytesRead metric to CPM (Closed)
|
Created:
8 years, 4 months ago by Devlin Modified:
8 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd BytesRead metric to CPM
Add watching for how many bytes are read. Useful for future analytics, and to
see periods of increased downloads.
BUG=130212
TEST=Included browsertest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153553
Patch Set 1 #
Total comments: 13
Patch Set 2 : Requested changes + separation for disk/network reads #
Total comments: 13
Patch Set 3 : Threading Changes #
Total comments: 6
Patch Set 4 : Requested changes #
Total comments: 8
Patch Set 5 : Moved initialized check to PerformanceMonitor #Patch Set 6 : Latest master for cq #Patch Set 7 : Removed const char[]s so that FILE_PATH_LITERAL and append both work #
Messages
Total messages: 25 (0 generated)
LGTM https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:257: enabled_ = true; It might be a little misleading to have this be set late in the startup, and yet be called 'enabled', which seems like a property set at browser startup.
+ battre for c/b/net
https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:239: bytes_read); OnRawBytesRead is called on the IO thread. In PerformanceMonitor::GatherStatisticsOnBackgroundThread(), bytes_read is accessed on a background thread. https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:34: class PerformanceMonitor : public content::NotificationObserver { Can you document the thread-safety of this class? https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:148: // number of bytes that have been read since PerformanceMonitor first started "have been read from where"? net/disk/... https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:170: static bool enabled_; static attributes don't use the _ notation but use a g_ prefix. See "static int g_max_queueing_delay_ms = 0;" in predictor.cc. You would move the variable to the .cc file.
https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:386: void PerformanceMonitor::BytesRead(int bytes_read) { IO thread only? https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:170: static bool enabled_; On 2012/08/21 16:16:04, battre wrote: > static attributes don't use the _ notation but use a g_ prefix. See "static int > g_max_queueing_delay_ms = 0;" in predictor.cc. You would move the variable to > the .cc file. Isn't the g_ prefix only for global data? I think I have seen this pattern before: http://code.google.com/searchframe#OAMlx_jo-ck/src/base/time.h&exact_package=... http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/jankometer....
Next round. Yoyo, there may be enough of a delta since your review to warrant a second quick check. https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:239: bytes_read); On 2012/08/21 16:16:04, battre wrote: > OnRawBytesRead is called on the IO thread. In > PerformanceMonitor::GatherStatisticsOnBackgroundThread(), bytes_read is accessed > on a background thread. See comment in performance_monitor.cc https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:257: enabled_ = true; On 2012/08/18 01:48:56, Yoyo Zhou wrote: > It might be a little misleading to have this be set late in the startup, and yet > be called 'enabled', which seems like a property set at browser startup. Done. https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:386: void PerformanceMonitor::BytesRead(int bytes_read) { On 2012/08/21 17:17:26, eaugusti wrote: > IO thread only? Done. https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:34: class PerformanceMonitor : public content::NotificationObserver { On 2012/08/21 16:16:04, battre wrote: > Can you document the thread-safety of this class? Done (and very needed, thank you). Please let me know if this makes sense, or if I should be more descriptive about any part. https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:148: // number of bytes that have been read since PerformanceMonitor first started On 2012/08/21 16:16:04, battre wrote: > "have been read from where"? net/disk/... Good point. I found this through TaskManager's code, and didn't think to check that the Network column would refer to multiple places (but it does). This has now been separated into two metrics. Thanks :) https://chromiumcodereview.appspot.com/10829342/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:170: static bool enabled_; On 2012/08/21 17:17:26, eaugusti wrote: > On 2012/08/21 16:16:04, battre wrote: > > static attributes don't use the _ notation but use a g_ prefix. See "static > int > > g_max_queueing_delay_ms = 0;" in predictor.cc. You would move the variable to > > the .cc file. > > Isn't the g_ prefix only for global data? > > I think I have seen this pattern before: > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/time.h&exact_package=... > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/jankometer.... Other examples: http://code.google.com/searchframe#OAMlx_jo-ck/src/base/metrics/statistics_re... http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/browsing_da... (examples where a static bool is returned in a accessor function)
https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:272: base::Int64ToString(bytes_read_.disk)); If you access bytes_read_ here, you must be on the IO thread. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:60: }; I haven't read all the code, but I would probably implement this the following way: struct PerformanceDataForIOThread { int64 network; } PerformanceDataForIOThread performance_data_for_io_thread_; BytesRead() modifies performance_data_for_io_thread_ directly. To gather this metric for reporting, you would post a task to the IOThread that posts a callback tasks with a copy of the data structure to the background thread in order to persist it. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:81: // accordingly. Sorry for my imprecise comment. I was asking to extend the comment with a description that this is about number of bytes received from the network. Usually system monitors distinguish between network traffic and disk traffic. The latter would account for cache accesses, database operations, etc. I think I would either drop the ByteCount::disk metric for now or try to catch all disk access. This way it reports incomplete numbers.
https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:58: int64 disk; Should these be uint64?
https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:51: // thread). When performing an action which should be done on a certain thread, You don't need this bit about DCHECKing threads; it's well-understood and obvious. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:81: // accordingly. On 2012/08/21 20:37:33, battre wrote: > Sorry for my imprecise comment. I was asking to extend the comment with a > description that this is about number of bytes received from the network. > > Usually system monitors distinguish between network traffic and disk traffic. > The latter would account for cache accesses, database operations, etc. I think I > would either drop the ByteCount::disk metric for now or try to catch all disk > access. This way it reports incomplete numbers. Ooh, what about network file systems? file:/// URLs can still fetch things from the network.
https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:60: }; On 2012/08/21 20:37:33, battre wrote: > I haven't read all the code, but I would probably implement this the following > way: > > struct PerformanceDataForIOThread { > int64 network; > } > PerformanceDataForIOThread performance_data_for_io_thread_; > > BytesRead() modifies performance_data_for_io_thread_ directly. > > To gather this metric for reporting, you would post a task to the IOThread that > posts a callback tasks with a copy of the data structure to the background > thread in order to persist it. I understand your suggestion, but am still confused as to why it's necessary... - bytes_read_ can only be modified on one thread, IO (no assignment race condition). - if the io thread is updating bytes read, we don't care if we insert the old value into the database, since the new value will be inserted in the next iteration, and it was close enough to be justified in either time slot. - the data structure will persist so long as PerformanceMonitor lives (and if PerformanceMonitor dies, so does the database, and the insertion is moot anyway). I have no problem making the change, but would like to understand the rationale behind it.
https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:60: }; On 2012/08/21 21:49:02, D Cronin wrote: > On 2012/08/21 20:37:33, battre wrote: > > I haven't read all the code, but I would probably implement this the following > > way: > > > > struct PerformanceDataForIOThread { > > int64 network; > > } > > PerformanceDataForIOThread performance_data_for_io_thread_; > > > > BytesRead() modifies performance_data_for_io_thread_ directly. > > > > To gather this metric for reporting, you would post a task to the IOThread > that > > posts a callback tasks with a copy of the data structure to the background > > thread in order to persist it. > > I understand your suggestion, but am still confused as to why it's necessary... > - bytes_read_ can only be modified on one thread, IO (no assignment race > condition). > - if the io thread is updating bytes read, we don't care if we insert the old > value into the database, since the new value will be inserted in the next > iteration, and it was close enough to be justified in either time slot. > - the data structure will persist so long as PerformanceMonitor lives (and if > PerformanceMonitor dies, so does the database, and the insertion is moot > anyway). > > I have no problem making the change, but would like to understand the rationale > behind it. Ignore that. Implementing change. (*grumbles something about using half-updated values*)
Round 3. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:272: base::Int64ToString(bytes_read_.disk)); On 2012/08/21 20:37:33, battre wrote: > If you access bytes_read_ here, you must be on the IO thread. Done. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:51: // thread). When performing an action which should be done on a certain thread, On 2012/08/21 21:29:16, Yoyo Zhou wrote: > You don't need this bit about DCHECKing threads; it's well-understood and > obvious. Done. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:58: int64 disk; On 2012/08/21 21:19:32, eaugusti wrote: > Should these be uint64? Done. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:60: }; On 2012/08/21 22:10:33, D Cronin wrote: > On 2012/08/21 21:49:02, D Cronin wrote: > > On 2012/08/21 20:37:33, battre wrote: > > > I haven't read all the code, but I would probably implement this the > following > > > way: > > > > > > struct PerformanceDataForIOThread { > > > int64 network; > > > } > > > PerformanceDataForIOThread performance_data_for_io_thread_; > > > > > > BytesRead() modifies performance_data_for_io_thread_ directly. > > > > > > To gather this metric for reporting, you would post a task to the IOThread > > that > > > posts a callback tasks with a copy of the data structure to the background > > > thread in order to persist it. > > > > I understand your suggestion, but am still confused as to why it's > necessary... > > - bytes_read_ can only be modified on one thread, IO (no assignment race > > condition). > > - if the io thread is updating bytes read, we don't care if we insert the old > > value into the database, since the new value will be inserted in the next > > iteration, and it was close enough to be justified in either time slot. > > - the data structure will persist so long as PerformanceMonitor lives (and if > > PerformanceMonitor dies, so does the database, and the insertion is moot > > anyway). > > > > I have no problem making the change, but would like to understand the > rationale > > behind it. > > Ignore that. Implementing change. (*grumbles something about using half-updated > values*) Done. https://chromiumcodereview.appspot.com/10829342/diff/9003/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:81: // accordingly. On 2012/08/21 20:37:33, battre wrote: > Sorry for my imprecise comment. I was asking to extend the comment with a > description that this is about number of bytes received from the network. > > Usually system monitors distinguish between network traffic and disk traffic. > The latter would account for cache accesses, database operations, etc. I think I > would either drop the ByteCount::disk metric for now or try to catch all disk > access. This way it reports incomplete numbers. Got rid of the disk metric, as requested. Per Yoyo's comment, is there a way of distinguishing a network file system access from any other file access (I assume that should still fall into network traffic)?
https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:408: const PerformanceDataForIOThread& performance_data_for_io_thread) { you need to pass a copy instead of a const ref here to be thread safe. https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:754: Database::MetricInfoVector metrics = GetStats(METRIC_NETWORK_BYTES_READ); It this still correct? (just a question, I have not checked it myself) GetStats flushes the queues in some order and I wonder whether this ensures that all messages are sent and guaranteed to be executed in the right order.
https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:408: const PerformanceDataForIOThread& performance_data_for_io_thread) { On 2012/08/22 08:25:24, battre wrote: > you need to pass a copy instead of a const ref here to be thread safe. Done. https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:754: Database::MetricInfoVector metrics = GetStats(METRIC_NETWORK_BYTES_READ); On 2012/08/22 08:25:24, battre wrote: > It this still correct? (just a question, I have not checked it myself) GetStats > flushes the queues in some order and I wonder whether this ensures that all > messages are sent and guaranteed to be executed in the right order. Done, added in an extra flush to be safe.
https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:754: Database::MetricInfoVector metrics = GetStats(METRIC_NETWORK_BYTES_READ); On 2012/08/22 16:22:05, D Cronin wrote: > On 2012/08/22 08:25:24, battre wrote: > > It this still correct? (just a question, I have not checked it myself) > GetStats > > flushes the queues in some order and I wonder whether this ensures that all > > messages are sent and guaranteed to be executed in the right order. > > Done, added in an extra flush to be safe. Did you check whether it was needed? https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:110: performance_data_for_io_thread_.network_bytes_read = info.value; you access the variable on the UI thread. Sorry that I did not notice this earlier.
https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/10007/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:754: Database::MetricInfoVector metrics = GetStats(METRIC_NETWORK_BYTES_READ); On 2012/08/22 19:23:02, battre wrote: > On 2012/08/22 16:22:05, D Cronin wrote: > > On 2012/08/22 08:25:24, battre wrote: > > > It this still correct? (just a question, I have not checked it myself) > > GetStats > > > flushes the queues in some order and I wonder whether this ensures that all > > > messages are sent and guaranteed to be executed in the right order. > > > > Done, added in an extra flush to be safe. > > Did you check whether it was needed? Yeah, it's needed to ensure that it will be done (until we move everything to the blocking pool - but I have no idea if/when that will happen, or even if it's still planned). Thanks for catching it :) https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:110: performance_data_for_io_thread_.network_bytes_read = info.value; On 2012/08/22 19:23:02, battre wrote: > you access the variable on the UI thread. Sorry that I did not notice this > earlier. Well, I've been wrong on threading every step of the way here, but I still *think* this one's okay (I look forward to the reason I'm wrong!) :) This happens (only) during the initialization of PerformanceMonitor, which is, in itself, synchronous (it has thread jumping, but it does so synchronously). Only at the *end* of the initialization is the initialized_ flag set to true. OnRawBytesRead() checks this initialized flag to make sure that PerformanceMonitor is initialized before calling BytesReadOnIOThread(). Thus, we only call BytesReadOnIOThread() if we have conclusively already done the access on this thread. initialized_ isn't a thread-safety concern because it is a boolean - it is either on or off. If it is even partially on , then the instruction was reached and it is, in effect, fully on. Otherwise it is off, and should be off.
LGTM after fixing final comments and if the try bots are happy. https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:110: performance_data_for_io_thread_.network_bytes_read = info.value; On 2012/08/22 20:34:55, D Cronin wrote: > On 2012/08/22 19:23:02, battre wrote: > > you access the variable on the UI thread. Sorry that I did not notice this > > earlier. > > Well, I've been wrong on threading every step of the way here, but I still > *think* this one's okay (I look forward to the reason I'm wrong!) :) > > This happens (only) during the initialization of PerformanceMonitor, which is, > in itself, synchronous (it has thread jumping, but it does so synchronously). > Only at the *end* of the initialization is the initialized_ flag set to true. > OnRawBytesRead() checks this initialized flag to make sure that > PerformanceMonitor is initialized before calling BytesReadOnIOThread(). Thus, we > only call BytesReadOnIOThread() if we have conclusively already done the access > on this thread. > > initialized_ isn't a thread-safety concern because it is a boolean - it is > either on or off. If it is even partially on , then the instruction was reached > and it is, in effect, fully on. Otherwise it is off, and should be off. Maybe you can move the check whether initialzed_ is true to BytesReadOnIOThread(). That way the access is thread safe by the API of the class and does not depend on correct calling conventions. https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:763: ASSERT_GE(metrics[0].value, page1_size); I think this should be EXPECT_GE (not the ASSERT_EQ above, that is fine) https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:773: ASSERT_GE(metrics[1].value, page1_size + page2_size); I think this should be EXPECT_GE
https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:110: performance_data_for_io_thread_.network_bytes_read = info.value; On 2012/08/22 20:50:24, battre wrote: > On 2012/08/22 20:34:55, D Cronin wrote: > > On 2012/08/22 19:23:02, battre wrote: > > > you access the variable on the UI thread. Sorry that I did not notice this > > > earlier. > > > > Well, I've been wrong on threading every step of the way here, but I still > > *think* this one's okay (I look forward to the reason I'm wrong!) :) > > > > This happens (only) during the initialization of PerformanceMonitor, which is, > > in itself, synchronous (it has thread jumping, but it does so synchronously). > > Only at the *end* of the initialization is the initialized_ flag set to true. > > OnRawBytesRead() checks this initialized flag to make sure that > > PerformanceMonitor is initialized before calling BytesReadOnIOThread(). Thus, > we > > only call BytesReadOnIOThread() if we have conclusively already done the > access > > on this thread. > > > > initialized_ isn't a thread-safety concern because it is a boolean - it is > > either on or off. If it is even partially on , then the instruction was > reached > > and it is, in effect, fully on. Otherwise it is off, and should be off. > > Maybe you can move the check whether initialzed_ is true to > BytesReadOnIOThread(). That way the access is thread safe by the API of the > class and does not depend on correct calling conventions. Done. https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:763: ASSERT_GE(metrics[0].value, page1_size); On 2012/08/22 20:50:24, battre wrote: > I think this should be EXPECT_GE (not the ASSERT_EQ above, that is fine) Done. https://chromiumcodereview.appspot.com/10829342/diff/24001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:773: ASSERT_GE(metrics[1].value, page1_size + page2_size); On 2012/08/22 20:50:24, battre wrote: > I think this should be EXPECT_GE Done.
Yoyo and Eriq: Anything more?
Still LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10829342/2...
Try job failure for 10829342-29002 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10829342/1...
Change committed as 153553 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
