Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9732008:
[Sync] Store the past 10 traffic records in memory. (Closed)
|
Created:
8 years, 9 months ago by lipalani1 Modified:
8 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWe store the past 10 records of client server communication in a queue in memory.
The next patch would address the javascript side of exposing it in the UI.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130001
Patch Set 1 #Patch Set 2 : Self review. #Patch Set 3 : #Patch Set 4 : Self review. #Patch Set 5 : #Patch Set 6 : For review. #
Total comments: 17
Patch Set 7 : For review. #
Total comments: 2
Patch Set 8 : For review. #Patch Set 9 : For review. #Patch Set 10 : For review. #Patch Set 11 : For review. #Patch Set 12 : For review. #
Total comments: 14
Patch Set 13 : #Patch Set 14 : For review. #
Total comments: 11
Patch Set 15 : For review. #
Total comments: 11
Patch Set 16 : For review. #
Total comments: 6
Patch Set 17 : For review. #Patch Set 18 : For review. #
Total comments: 6
Patch Set 19 : For submitting. #Patch Set 20 : For submitting. #Patch Set 21 : For submitting. #
Messages
Total messages: 32 (0 generated)
Please review.
http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.cc File sync/engine/traffic_logger.cc (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:34: static const unsigned int kMaxMessageSize = 100 * 1024; 100k * 10 = 1MB, which seems like a lot just for logging. I was thinking something more on the order of 10k-50k total, So that would be max 1k-5k per message. Or you could just enforce the total size and let an individual message grow as large as it can. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:51: void AddTrafficToQueue(std::queue<TrafficRecord>* traffic_recorder, you'll run into problems later when reading the entries in the queue, since I believe you can only look at the top. You should just use the underlying data structure (a deque<>) and use push_back/pop_front. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:57: traffic_recorder->push(record); surely this should come before the popping? Otherwise, you effectively have a max count of kMaxMessages+1. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h File sync/engine/traffic_logger.h (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:39: public: no 'public'; everything in a struct is public by default http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:42: bool truncated); you should also define the default constructor (and initialize message_type/truncated) http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:46: void LogClientToServerMessage(const sync_pb::ClientToServerMessage& msg, hmm I don't think the storing logic belongs in this class. Since the queue is in the context object, why not add methods to it like 'RecordClientToServer{Message,Response}()'? Then either the caller can do: RecordClientToServerMessage(msg); LogClientToServerMessage(msg); or just have RecordClientToServerMessage call LogClientToServerMessage (my preference).
PTAL The next and final patch is ready at http://codereview.chromium.org/9826035/ which might explain how I am using the queue. Once this review is done I will send the review for that officially but feel free to take a look as needed. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.cc File sync/engine/traffic_logger.cc (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:34: static const unsigned int kMaxMessageSize = 100 * 1024; Letting the individual message size unbounded and having limit only on total has some issues. Eg: if a message is pretty close to the limit it would destroy the entire queue. Thus we have unpredictable number of messages in the queue. I think the solution would be trim the specifics if the message size is bigger. I have added the todo. On 2012/03/22 20:27:28, akalin wrote: > 100k * 10 = 1MB, which seems like a lot just for logging. I was thinking > something more on the order of 10k-50k total, So that would be max 1k-5k per > message. Or you could just enforce the total size and let an individual message > grow as large as it can. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:51: void AddTrafficToQueue(std::queue<TrafficRecord>* traffic_recorder, The next patch would only need to access the front of the queue and once it is displayed it is popped out. I dont think deque functionality is needed. On 2012/03/22 20:27:28, akalin wrote: > you'll run into problems later when reading the entries in the queue, since I > believe you can only look at the top. You should just use the underlying data > structure (a deque<>) and use push_back/pop_front. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:57: traffic_recorder->push(record); i did not mean >= in the previous while block. Changed it to just >. On 2012/03/22 20:27:28, akalin wrote: > surely this should come before the popping? Otherwise, you effectively have a > max count of kMaxMessages+1. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h File sync/engine/traffic_logger.h (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:39: public: On 2012/03/22 20:27:28, akalin wrote: > no 'public'; everything in a struct is public by default Done. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:42: bool truncated); On 2012/03/22 20:27:28, akalin wrote: > you should also define the default constructor (and initialize > message_type/truncated) Done. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:46: void LogClientToServerMessage(const sync_pb::ClientToServerMessage& msg, The context object for the most part has only getters. Take the examples of resolver(), directory(), registrar(), extensions_monitor() etc. It might make it cumbersome to add member methods here for each operation. One option that I can do is make the datastructure in context to be a TrafficLogger class which can have a public method called LogData which in turn calls private methods to log data locally and to memory. Let me know if you prefer that. What we have currently are global functions which (in my personal opinion) is sufficient enough for this light use. On 2012/03/22 20:27:28, akalin wrote: > hmm I don't think the storing logic belongs in this class. Since the queue is > in the context object, why not add methods to it like > 'RecordClientToServer{Message,Response}()'? Then either the caller can do: > > RecordClientToServerMessage(msg); > LogClientToServerMessage(msg); > > or just have RecordClientToServerMessage call LogClientToServerMessage (my > preference).
http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.cc File sync/engine/traffic_logger.cc (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:51: void AddTrafficToQueue(std::queue<TrafficRecord>* traffic_recorder, On 2012/03/23 00:03:11, lipalani1 wrote: > The next patch would only need to access the front of the queue and once it is > displayed it is popped out. I dont think deque functionality is needed. I see. But that's still not ideal. So if you do a dump and refresh about:sync (for whatever reason) you lose the data? That seems weird. It should probably be a read accessor and not a read-and-clear. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h File sync/engine/traffic_logger.h (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:46: void LogClientToServerMessage(const sync_pb::ClientToServerMessage& msg, On 2012/03/23 00:03:11, lipalani1 wrote: > The context object for the most part has only getters. Take the examples of > resolver(), directory(), registrar(), extensions_monitor() etc. It might make it > cumbersome to add member methods here for each operation. > > One option that I can do is make the datastructure in context to be a > TrafficLogger class which can have a public method called LogData which in turn > calls private methods to log data locally and to memory. > > Let me know if you prefer that. What we have currently are global functions > which (in my personal opinion) is sufficient enough for this light use. I think the problem is that you take 'log' to mean 'log and record', and I take it to mean just 'log'. I think it would be better to separate out the two concepts. A separate class would be best, actually, as it would encapsulate all the logic, instead of having it spread out across other files. Perhaps TrafficRecorder? http://codereview.chromium.org/9732008/diff/10001/sync/engine/traffic_logger.cc File sync/engine/traffic_logger.cc (right): http://codereview.chromium.org/9732008/diff/10001/sync/engine/traffic_logger.... sync/engine/traffic_logger.cc:58: while (traffic_recorder->size() > kMaxMessages) { actually, I think >= was correct. It's just confusing because usually you want to maintain invariants *after* you do an operation which may violate it. So // May violate the size invariant. traffic_recorder->push(record); // Maintain size invariant. while (traffic_recorder->size() > kMaxMessages) { traffic_record->pop(); } would be clearer than // Maintain a condition just similar enough to the invariant to be confusing. while (traffic_recorder->size() >= kMaxMessages) { traffic_record->pop(); } // This maintains the invariant, but you have to reason about the previous line to see it. traffic_recorder->push(record);
PTAL. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.cc File sync/engine/traffic_logger.cc (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.c... sync/engine/traffic_logger.cc:51: void AddTrafficToQueue(std::queue<TrafficRecord>* traffic_recorder, I did not want the memory to be used for displaying in JS and us having a copy internally. But you are right because it is little weird and it is little hard to explain to all users. On 2012/03/23 00:58:50, akalin wrote: > On 2012/03/23 00:03:11, lipalani1 wrote: > > The next patch would only need to access the front of the queue and once it is > > displayed it is popped out. I dont think deque functionality is needed. > > I see. But that's still not ideal. So if you do a dump and refresh about:sync > (for whatever reason) you lose the data? That seems weird. It should probably > be a read accessor and not a read-and-clear. http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h File sync/engine/traffic_logger.h (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:46: void LogClientToServerMessage(const sync_pb::ClientToServerMessage& msg, On 2012/03/23 00:58:50, akalin wrote: > On 2012/03/23 00:03:11, lipalani1 wrote: > > The context object for the most part has only getters. Take the examples of > > resolver(), directory(), registrar(), extensions_monitor() etc. It might make > it > > cumbersome to add member methods here for each operation. > > > > One option that I can do is make the datastructure in context to be a > > TrafficLogger class which can have a public method called LogData which in > turn > > calls private methods to log data locally and to memory. > > > > Let me know if you prefer that. What we have currently are global functions > > which (in my personal opinion) is sufficient enough for this light use. > > I think the problem is that you take 'log' to mean 'log and record', and I take > it to mean just 'log'. I think it would be better to separate out the two > concepts. > > A separate class would be best, actually, as it would encapsulate all the logic, > instead of having it spread out across other files. Perhaps TrafficRecorder? Done. http://codereview.chromium.org/9732008/diff/10001/sync/engine/traffic_logger.cc File sync/engine/traffic_logger.cc (right): http://codereview.chromium.org/9732008/diff/10001/sync/engine/traffic_logger.... sync/engine/traffic_logger.cc:58: while (traffic_recorder->size() > kMaxMessages) { On 2012/03/23 00:58:50, akalin wrote: > actually, I think >= was correct. It's just confusing because usually you want > to maintain invariants *after* you do an operation which may violate it. So > > // May violate the size invariant. > traffic_recorder->push(record); > // Maintain size invariant. > while (traffic_recorder->size() > kMaxMessages) { > traffic_record->pop(); > } > > would be clearer than > > // Maintain a condition just similar enough to the invariant to be confusing. > while (traffic_recorder->size() >= kMaxMessages) { > traffic_record->pop(); > } > // This maintains the invariant, but you have to reason about the previous line > to see it. > traffic_recorder->push(record); Done.
http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h File sync/engine/traffic_logger.h (right): http://codereview.chromium.org/9732008/diff/2010/sync/engine/traffic_logger.h... sync/engine/traffic_logger.h:46: void LogClientToServerMessage(const sync_pb::ClientToServerMessage& msg, On 2012/03/26 21:25:21, lipalani1 wrote: > On 2012/03/23 00:58:50, akalin wrote: > > On 2012/03/23 00:03:11, lipalani1 wrote: > > > The context object for the most part has only getters. Take the examples of > > > resolver(), directory(), registrar(), extensions_monitor() etc. It might > make > > it > > > cumbersome to add member methods here for each operation. > > > > > > One option that I can do is make the datastructure in context to be a > > > TrafficLogger class which can have a public method called LogData which in > > turn > > > calls private methods to log data locally and to memory. > > > > > > Let me know if you prefer that. What we have currently are global functions > > > which (in my personal opinion) is sufficient enough for this light use. > > > > I think the problem is that you take 'log' to mean 'log and record', and I > take > > it to mean just 'log'. I think it would be better to separate out the two > > concepts. > > > > A separate class would be best, actually, as it would encapsulate all the > logic, > > instead of having it spread out across other files. Perhaps TrafficRecorder? > > Done. I meant a separate class for *just* storing/recording the traffic. You haven't addressed my main concern, which is to separate out the two concepts. Basically, you should leave traffic_logger.{h,cc} as is (since otherwise you'd have to change the switch), and have traffic_recorder.{h,cc} store/record the traffic and do nothing else. The caller can just call into the recorder and logger at the same time.
PTAL.
more comments http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:7: #include <queue> omit <queue> (not needed anymore) omit <string> (included from header) http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:20: using sessions::SyncSession; not needed naymore http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:53: while (records_.size() >= browser_sync::kMaxMessages) { this is wrong -- please see my previous comment again. >= should be > http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorder.h File sync/engine/traffic_recorder.h (right): http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:5: // This file has the functions to log all the sync related HTTP communication. fix this comment http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:36: // If the message is too big to be kept in memory then it would be trucated. would be trucated -> should be truncated http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:37: // For now the entire message would be truncated if it is big. would be truncated -> is omitted is big -> is too big
http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:53: while (records_.size() >= browser_sync::kMaxMessages) { On 2012/03/27 20:21:04, akalin wrote: > this is wrong -- please see my previous comment again. > > >= should be > quoting mangled my 2nd line -- should be: "The >= should be >" I think the fact that this hasn't been correct for quite a few patch sets means that we need unit tests for TrafficRecorder. :)
PTAL. Includes tests. http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:7: #include <queue> On 2012/03/27 20:21:04, akalin wrote: > omit <queue> (not needed anymore) > omit <string> (included from header) Done. http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:20: using sessions::SyncSession; On 2012/03/27 20:21:04, akalin wrote: > not needed naymore Done. http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:53: while (records_.size() >= browser_sync::kMaxMessages) { On 2012/03/27 20:21:04, akalin wrote: > this is wrong -- please see my previous comment again. > > >= should be > Done. http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:53: while (records_.size() >= browser_sync::kMaxMessages) { On 2012/03/27 20:21:04, akalin wrote: > this is wrong -- please see my previous comment again. > > >= should be > Done. http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorder.h File sync/engine/traffic_recorder.h (right): http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:5: // This file has the functions to log all the sync related HTTP communication. On 2012/03/27 20:21:04, akalin wrote: > fix this comment Done. http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:36: // If the message is too big to be kept in memory then it would be trucated. On 2012/03/27 20:21:04, akalin wrote: > would be trucated -> should be truncated Done. http://codereview.chromium.org/9732008/diff/20001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:37: // For now the entire message would be truncated if it is big. On 2012/03/27 20:21:04, akalin wrote: > would be truncated -> is omitted > is big -> is too big Done.
More comments. No need to rush, just make sure to get it right. http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorder.h File sync/engine/traffic_recorder.h (right): http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:57: FRIEND_TEST_ALL_PREFIXES(TrafficRecorderTest, MaxRecordsTest); I prefer doing read accessors over giving tests access to innards. You're going to have to add accessors later anyway, since you have to get the data out somehow, right? http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:63: std::deque<TrafficRecord> records_; DISALLOW_COPY_AND_ASSIGN() (don't forget to include base/basictypes.h) http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... File sync/engine/traffic_recorder_unittest.cc (right): http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:12: extern const unsigned int kMaxMessages; eh, this is pretty gross. :/ Make TrafficRecorder's constructor take these as parameters, and move the constants in SyncSession or something. Then you can test with some constants for test purposes. also, why would you need to re-declare these again here if you're including the header file (which has these declarations)? http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:20: for (unsigned int i = 0; i<2*kMaxMessages; ++i) prefer calling the public interface http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:31: while ((unsigned int)response.ByteSize() < kMaxMessageSize) { you can just assign to error_message some large string
Richard - since Fred is in vacation I think you are the best person to continue this review. You can look at patch 14 and Fred's comments and my answers and patch 15. The patches before that were mostly for self review or patches in which we were discussing the basic design. Basically we are storing client server traffic to display in about:sync. For reference that patch is here: http://codereview.chromium.org/9826035/ (not rebased yet) If you have a question about the design you can feel free to ping me as that would be faster or you can go through all the past discussions. http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorder.h File sync/engine/traffic_recorder.h (right): http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:57: FRIEND_TEST_ALL_PREFIXES(TrafficRecorderTest, MaxRecordsTest); On 2012/03/27 23:53:44, akalin wrote: > I prefer doing read accessors over giving tests access to innards. You're going > to have to add accessors later anyway, since you have to get the data out > somehow, right? Done. http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:63: std::deque<TrafficRecord> records_; On 2012/03/27 23:53:44, akalin wrote: > DISALLOW_COPY_AND_ASSIGN() > > (don't forget to include base/basictypes.h) Done. http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... File sync/engine/traffic_recorder_unittest.cc (right): http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:12: extern const unsigned int kMaxMessages; On 2012/03/27 23:53:44, akalin wrote: > eh, this is pretty gross. :/ Make TrafficRecorder's constructor take these as > parameters, and move the constants in SyncSession or something. Then you can > test with some constants for test purposes. > > also, why would you need to re-declare these again here if you're including the > header file (which has these declarations)? Done. http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:20: for (unsigned int i = 0; i<2*kMaxMessages; ++i) On 2012/03/27 23:53:44, akalin wrote: > prefer calling the public interface Done. http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:31: while ((unsigned int)response.ByteSize() < kMaxMessageSize) { hmm... I dont want to type in a large error message. This seems easier. On 2012/03/27 23:53:44, akalin wrote: > you can just assign to error_message some large string
http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... File sync/engine/traffic_recorder_unittest.cc (right): http://codereview.chromium.org/9732008/diff/20002/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:31: while ((unsigned int)response.ByteSize() < kMaxMessageSize) { On 2012/03/29 00:23:13, lipalani1 wrote: > hmm... I dont want to type in a large error message. This seems easier. > On 2012/03/27 23:53:44, akalin wrote: > > you can just assign to error_message some large string You don't have to literally type in the large error message. You can just do: std::string foo(kMaxMessageSize); and it'll initialize a string of that size.
Here are my comments. Though it seems Fred might still be the main reviewer. Feel free to ignore the efficiency related comments. The cost of some inefficiency in this code is unlikely to be anywhere near as expensive as the network IO this is intended to record. If it's convenient, though, it would be nice to avoid some of those allocs and memcpy()s. http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:65: TrafficRecord record(message, type, truncated); Unless the compiler is really smart, this will likely be another copy. You could avoid this if you cad a TrafficRecord constructor that took a const MessageLite& parameter and serialized to its member directly. http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorder.h File sync/engine/traffic_recorder.h (right): http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:27: CLIENT_TO_SERVER_RESPONSE, Shouldn't at least one of these be SERVER_TO_CLIENT? http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:37: // TODO(lipalani): Truncate the specifics to fit with in size. nit: s/with in/within/ http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:68: std::deque<TrafficRecord> records_; I normally try to avoid premature optimization, but this is a bit excessive even by my standards. Is it really necessary to copy construct these large strings every time we push or pop an item?
PTAL. http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:65: TrafficRecord record(message, type, truncated); this is only one extra copy. We dont copy it during accessing. It will become clear in the next CR. Also the only way to implement this would be smart pointers. I am deferring it until a need arises. On 2012/03/29 17:56:21, rlarocque wrote: > Unless the compiler is really smart, this will likely be another copy. You > could avoid this if you cad a TrafficRecord constructor that took a const > MessageLite& parameter and serialized to its member directly. http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorder.h File sync/engine/traffic_recorder.h (right): http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:27: CLIENT_TO_SERVER_RESPONSE, Ideally yes. But the proto is named this way and there is too much stuff built around this name. On 2012/03/29 17:56:21, rlarocque wrote: > Shouldn't at least one of these be SERVER_TO_CLIENT? http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:37: // TODO(lipalani): Truncate the specifics to fit with in size. On 2012/03/29 17:56:21, rlarocque wrote: > nit: s/with in/within/ Done. http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.h:68: std::deque<TrafficRecord> records_; answered in the next comment. Bottom line this would involve only one extra copying. On 2012/03/29 17:56:21, rlarocque wrote: > I normally try to avoid premature optimization, but this is a bit excessive even > by my standards. > > Is it really necessary to copy construct these large strings every time we push > or pop an item?
Couldn't resist a few more suggestions, but I leave the final review to Richard. http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:58: if ((unsigned int)msg.ByteSize() >= max_message_size_) { static_cast<unsigned int> http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:65: TrafficRecord record(message, type, truncated); No, smart pointers isn't the only way. Here's an easy way that should make everyone happy: Make AddTrafficToQueue look like: void ...AddTrafficToQueue(TrafficRecord* record) { records_.resize(records_.size() + 1); std::swap(records_.back(), *record); ... } The string swap should be efficient. On 2012/03/29 18:39:29, lipalani1 wrote: > this is only one extra copy. We dont copy it during accessing. It will become > clear in the next CR. Also the only way to implement this would be smart > pointers. I am deferring it until a need arises. > On 2012/03/29 17:56:21, rlarocque wrote: > > Unless the compiler is really smart, this will likely be another copy. You > > could avoid this if you cad a TrafficRecord constructor that took a const > > MessageLite& parameter and serialized to its member directly. > http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... File sync/engine/traffic_recorder_unittest.cc (right): http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:17: TrafficRecorder recorder(kMaxMessages,kMaxMessageSize); space after comma http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:31: while ((unsigned int)response.ByteSize() < kMaxMessageSize) { I still prefer the one-liner to add a large string for the error (see my previous comment) http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:36: TrafficRecorder recorder(kMaxMessages,kMaxMessageSize); space after comma
PTAL. All comments fixed. Fred - cool idea and it is implemented. The one liner string in unit test was fixed. I sent a note via email but for some reason it did not appear on the tool. Feel free to send more suggestions. Richard - Feel free to review and LGTM as you see fit. I will coordinate with Fred seperately if needed. http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... File sync/engine/traffic_recorder_unittest.cc (right): http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:17: TrafficRecorder recorder(kMaxMessages,kMaxMessageSize); On 2012/03/29 18:54:06, akalin wrote: > space after comma Done. http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:31: while ((unsigned int)response.ByteSize() < kMaxMessageSize) { On 2012/03/29 18:54:06, akalin wrote: > I still prefer the one-liner to add a large string for the error (see my > previous comment) Done. http://codereview.chromium.org/9732008/diff/30003/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:36: TrafficRecorder recorder(kMaxMessages,kMaxMessageSize); On 2012/03/29 18:54:06, akalin wrote: > space after comma Done.
LGTM! http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:59: if ((unsigned int)msg.ByteSize() >= max_message_size_) { static_cast<> here http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... File sync/engine/traffic_recorder_unittest.cc (right): http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:20: for (unsigned int i = 0; i<2*kMaxMessages; ++i) spaces around '<'
http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:65: TrafficRecord record(message, type, truncated); On 2012/03/29 18:54:06, akalin wrote: > No, smart pointers isn't the only way. Here's an easy way that should make > everyone happy: > > Make AddTrafficToQueue look like: > > void ...AddTrafficToQueue(TrafficRecord* record) { > records_.resize(records_.size() + 1); > std::swap(records_.back(), *record); > ... > } > > The string swap should be efficient. > That's a clever solution, but I think it requires either a template specialization of swap<TrafficRecord> or compiler support for std::move for this to be effective. Otherwise the swap will be implemented as { t = a; a = b; b = t; } with the default assignment operators. My suggestion for this particular case was to pass the proto directly into the constructor so it could be serialized into the TrafficRecord's member, rather than having it serialized into the stack-allocated temporary "message" then having the result copied into the TrafficRecord's member as that object is constructed. No smart pointers required. I suppose you could swap() the message argument into the member during construction, but that seems like bad style. It's not obvious to the caller that the constructor would modify one of its arguments. I didn't intend to spend a lot of time on this. My point was that there exists an alternate way to write this code that is probably more efficient but otherwise very similar. It might be worth making the change if doing so doesn't require a lot of effort and doesn't obfuscate the code too much.
ah.. Richard you are right. For some reason I thought swap would call string's swap method but looking at the swap code it just does copy to temp and copy back which is not a real swap and not any better than what we had before :( (also it is OK to spend some time on this): I was thinking about changing to what you suggested before Fred's suggestion. The problem with that approach is we will have to pass in the max_message_size as a parameter to the TrafficRecord constructor along with IMessage. That variable does not seem to belong there(it is not like different records have different sizes)... On 2012/03/29 21:38:56, rlarocque wrote: > http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... > File sync/engine/traffic_recorder.cc (right): > > http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... > sync/engine/traffic_recorder.cc:65: TrafficRecord record(message, type, > truncated); > On 2012/03/29 18:54:06, akalin wrote: > > No, smart pointers isn't the only way. Here's an easy way that should make > > everyone happy: > > > > Make AddTrafficToQueue look like: > > > > void ...AddTrafficToQueue(TrafficRecord* record) { > > records_.resize(records_.size() + 1); > > std::swap(records_.back(), *record); > > ... > > } > > > > The string swap should be efficient. > > > > That's a clever solution, but I think it requires either a template > specialization of swap<TrafficRecord> or compiler support for std::move for this > to be effective. Otherwise the swap will be implemented as { t = a; a = b; b = > t; } with the default assignment operators. > > My suggestion for this particular case was to pass the proto directly into the > constructor so it could be serialized into the TrafficRecord's member, rather > than having it serialized into the stack-allocated temporary "message" then > having the result copied into the TrafficRecord's member as that object is > constructed. No smart pointers required. > > I suppose you could swap() the message argument into the member during > construction, but that seems like bad style. It's not obvious to the caller > that the constructor would modify one of its arguments. > > I didn't intend to spend a lot of time on this. My point was that there exists > an alternate way to write this code that is probably more efficient but > otherwise very similar. It might be worth making the change if doing so doesn't > require a lot of effort and doesn't obfuscate the code too much.
http://codereview.chromium.org/9732008/diff/29017/sync/sessions/sync_session_... File sync/sessions/sync_session_context.h (right): http://codereview.chromium.org/9732008/diff/29017/sync/sessions/sync_session_... sync/sessions/sync_session_context.h:185: browser_sync::TrafficRecorder traffic_recorder_; Please see my comment on http://codereview.chromium.org/9826035/. I think we should follow the pattern here and have this object live at the top of the stack of where it is used (in SyncManager) to avoid circuitous accesses of the context from (e.g. the scheduler).
On 2012/03/29 21:55:55, lipalani1 wrote: > ah.. Richard you are right. For some reason I thought swap would call string's > swap method but looking at the swap code it just does copy to temp and copy back > which is not a real swap and not any better than what we had before :( FWIW, glibc's version is a bit smarter, but only in C++11 mode. The compilers of the future will make this easier to implement. > (also it is OK to spend some time on this): > > I was thinking about changing to what you suggested before Fred's suggestion. > The problem with that approach is we will have to pass in the max_message_size > as a parameter to the TrafficRecord constructor along with IMessage. That > variable does not seem to belong there(it is not like different records have > different sizes)... In that case, one alternative would be to create a static factory function that takes in the max_message_parameter and calls one of several possible constructors based on whether or not truncation is necessary. However, unless we want to resort to some deep C++ magic, this function would return a pointer to a recently new'ed TrafficRecord. If you were planning to store pointers to traffic records, this would be no problem. Because that's currently not the plan, it is a problem. That's the cleanest solution I can think of. If you feel like implementing it, go ahead. However, like I mentioned before, it's likely that we can afford the performance hit. > On 2012/03/29 21:38:56, rlarocque wrote: > > > http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... > > File sync/engine/traffic_recorder.cc (right): > > > > > http://codereview.chromium.org/9732008/diff/28001/sync/engine/traffic_recorde... > > sync/engine/traffic_recorder.cc:65: TrafficRecord record(message, type, > > truncated); > > On 2012/03/29 18:54:06, akalin wrote: > > > No, smart pointers isn't the only way. Here's an easy way that should make > > > everyone happy: > > > > > > Make AddTrafficToQueue look like: > > > > > > void ...AddTrafficToQueue(TrafficRecord* record) { > > > records_.resize(records_.size() + 1); > > > std::swap(records_.back(), *record); > > > ... > > > } > > > > > > The string swap should be efficient. > > > > > > > That's a clever solution, but I think it requires either a template > > specialization of swap<TrafficRecord> or compiler support for std::move for > this > > to be effective. Otherwise the swap will be implemented as { t = a; a = b; b > = > > t; } with the default assignment operators. > > > > My suggestion for this particular case was to pass the proto directly into the > > constructor so it could be serialized into the TrafficRecord's member, rather > > than having it serialized into the stack-allocated temporary "message" then > > having the result copied into the TrafficRecord's member as that object is > > constructed. No smart pointers required. > > > > I suppose you could swap() the message argument into the member during > > construction, but that seems like bad style. It's not obvious to the caller > > that the constructor would modify one of its arguments. > > > > I didn't intend to spend a lot of time on this. My point was that there > exists > > an alternate way to write this code that is probably more efficient but > > otherwise very similar. It might be worth making the change if doing so > doesn't > > require a lot of effort and doesn't obfuscate the code too much.
Comments addressed and submitting. I wait until later tonight or Tim responds before hitting the commit button to make sure he is ok with my explanation. http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... File sync/engine/traffic_recorder.cc (right): http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... sync/engine/traffic_recorder.cc:59: if ((unsigned int)msg.ByteSize() >= max_message_size_) { On 2012/03/29 19:46:36, akalin wrote: > static_cast<> here Done. http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... File sync/engine/traffic_recorder_unittest.cc (right): http://codereview.chromium.org/9732008/diff/29017/sync/engine/traffic_recorde... sync/engine/traffic_recorder_unittest.cc:20: for (unsigned int i = 0; i<2*kMaxMessages; ++i) On 2012/03/29 19:46:36, akalin wrote: > spaces around '<' Done. http://codereview.chromium.org/9732008/diff/29017/sync/sessions/sync_session_... File sync/sessions/sync_session_context.h (right): http://codereview.chromium.org/9732008/diff/29017/sync/sessions/sync_session_... sync/sessions/sync_session_context.h:185: browser_sync::TrafficRecorder traffic_recorder_; Tim - this is fixed in the next patch. In this patch it is not used elsewhere and scheduler does not expose it either. So no layering violations. I have left a todo here which will be fixed in the next patch. Hope that is OK. On 2012/03/29 22:02:23, timsteele wrote: > Please see my comment on http://codereview.chromium.org/9826035/. I think we > should follow the pattern here and have this object live at the top of the stack > of where it is used (in SyncManager) to avoid circuitous accesses of the context > from (e.g. the scheduler).
Yup, LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lipalani@chromium.org/9732008/40001
Try job failure for 9732008-40001 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
On 2012/03/29 21:38:56, rlarocque wrote:
> That's a clever solution, but I think it requires either a template
> specialization of swap<TrafficRecord> or compiler support for std::move for
this
> to be effective. Otherwise the swap will be implemented as { t = a; a = b; b
=
> t; } with the default assignment operators.
D'oh! Yeah, you'd need to define swap for TrafficRecord. That's not terrible,
though, is it?
struct TrafficRecord {
void Swap(TrafficRecord* r) {
using std::swap;
swap(foo, r->foo);
...
}
}
...
records.resize(records.size() + 1);
records.back().Swap(new_record);
(a Swap member function is preferable to overloading swap(), which would require
non-const refs)
This can be done in a future CL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lipalani@chromium.org/9732008/50002
Try job failure for 9732008-50002 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lipalani@chromium.org/9732008/55003
Change committed as 130001
Note that traffic_recorder_unittest.cc was checked in with the executable bit set, which caused this buildbot failure: http://build.chromium.org/p/chromium/builders/Linux/builds/21881/steps/check_... Fixed with http://crrev.com/130015 Mihai On Fri, Mar 30, 2012 at 5:03 PM, <commit-bot@chromium.org> wrote: > Change committed as 130001 > > http://codereview.chromium.**org/9732008/<http://codereview.chromium.org/9732... > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
