Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13473005:
Adding partially circular memory buffer wrapper. (Closed)
|
Created:
7 years, 8 months ago by Henrik Grunell Modified:
7 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding partially circular memory buffer wrapper.
Will be used by WebRTC logging.
BUG=229829
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195137
Patch Set 1 #
Total comments: 30
Patch Set 2 : Code review + added some lost code. #
Total comments: 12
Patch Set 3 : Code review #Patch Set 4 : Renamed files to match new class name. #
Total comments: 2
Patch Set 5 : Added unit test. Code review fix. Fixed some errors. Fixed dchecks. #Patch Set 6 : Fixed compilation error on some setups. #
Total comments: 17
Patch Set 7 : Code review fixes. Removed packing of struct. #
Total comments: 8
Patch Set 8 : Code review. #Patch Set 9 : Changed size_t to uint32 #Patch Set 10 : Re-added packing of struct. #
Total comments: 2
Patch Set 11 : Code review and rebase. #
Total comments: 2
Patch Set 12 : Code review + added CONTENT_EXPORT #Patch Set 13 : Rebase #
Messages
Total messages: 22 (0 generated)
Unit test tbd.
drive by https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... File content/common/circular_memory_buffer.cc (right): https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:10: ((a) < (b) ? ((a) < (c) ? (a) : (c)) : ((b) < (c) ? (b) : (c))) nit: std::min(a, std::min(b, c)) ? Since you only need this in a single place I'd prefer to just inline that code there instead of adding a macro. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:24: DCHECK_GT(memory_buffer_size_, 3 * sizeof(int32)); document this requirement? https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:29: position_ = 3; comment? https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:38: position_ = 3; seems like it would be good to have a well named constant for this value. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:47: int8* buffer_int8 = static_cast<int8*>(buffer); reinterpret_cast (void and int8 aren't related types) https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:80: size_t to_read = remaining_buffer_size < to_eof ? std::min? https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:99: size_t to_read = remaining_buffer_size < to_eob ? std::min https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:112: size_t to_write = buffer_size < to_eof ? buffer_size : to_eof; here as well (and elsewhere if there are more) https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:126: static_cast<const int8*>(buffer) + to_write, reinterpret_cast https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:137: total_written_ = static_cast<size_t>(*memory_buffer_int32); nit: what about using a struct or indices instead of incrementing? A struct would be nicer, but make sure that packing is not bigger than 4 bytes. If you use indices, the code will be slightly more readable imho: total_written_ = static_cast<size_t>(memory_buffer_int32[0]); wrap_position_ = static_cast<size_t>(memory_buffer_int32[1]); end_position_ = static_cast<size_t>(memory_buffer_int32[2]); But again, I think having a struct for the header would be better. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:147: int32* memory_buffer_int32 = reinterpret_cast<int32*>(memory_buffer_); same here and below https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... File content/common/circular_memory_buffer.h (right): https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:26: class CircularMemoryBuffer { Can we reflect the non-wrapping part better in the name? CircularMemoryBuffer suggests to me that it's a regular circular buffer. Having said that, nothing good comes to mind... PartialCircularBuffer? https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:31: void InitRead(); Could we have two separate constructors instead of init functions? I imagine that the InitRead() would take a const uint8* whereas the write constructor would accept uint8*. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:45: int8* memory_buffer_; nit: I think we tend to use uint8 for raw bytes of data. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:52: size_t total_written_; is total_written_ used for reading?
Done code review fixes. Also added some code changes that was lost before uploading patch 1. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... File content/common/circular_memory_buffer.cc (right): https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:10: ((a) < (b) ? ((a) < (c) ? (a) : (c)) : ((b) < (c) ? (b) : (c))) On 2013/04/02 19:34:08, tommi wrote: > nit: std::min(a, std::min(b, c)) ? Done. > Since you only need this in a single place I'd prefer to just inline that code > there instead of adding a macro. Hmm, the latest code changes I did before uploading got lost . I've re-added them, including two more places the macro is used. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:24: DCHECK_GT(memory_buffer_size_, 3 * sizeof(int32)); On 2013/04/02 19:34:08, tommi wrote: > document this requirement? Done. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:29: position_ = 3; On 2013/04/02 19:34:08, tommi wrote: > comment? I'm using sizeof on the new header struct to determine the header size. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:38: position_ = 3; On 2013/04/02 19:34:08, tommi wrote: > seems like it would be good to have a well named constant for this value. I'm using sizeof on the new header struct to determine the header size. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:47: int8* buffer_int8 = static_cast<int8*>(buffer); On 2013/04/02 19:34:08, tommi wrote: > reinterpret_cast (void and int8 aren't related types) Done. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:80: size_t to_read = remaining_buffer_size < to_eof ? On 2013/04/02 19:34:08, tommi wrote: > std::min? Changed to use MIN3 macro. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:99: size_t to_read = remaining_buffer_size < to_eob ? On 2013/04/02 19:34:08, tommi wrote: > std::min Changed to use MIN3 macro. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:112: size_t to_write = buffer_size < to_eof ? buffer_size : to_eof; On 2013/04/02 19:34:08, tommi wrote: > here as well (and elsewhere if there are more) Done. No other places. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:126: static_cast<const int8*>(buffer) + to_write, On 2013/04/02 19:34:08, tommi wrote: > reinterpret_cast Done. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:137: total_written_ = static_cast<size_t>(*memory_buffer_int32); On 2013/04/02 19:34:08, tommi wrote: > nit: what about using a struct or indices instead of incrementing? A struct > would be nicer, but make sure that packing is not bigger than 4 bytes. > If you use indices, the code will be slightly more readable imho: > total_written_ = static_cast<size_t>(memory_buffer_int32[0]); > wrap_position_ = static_cast<size_t>(memory_buffer_int32[1]); > end_position_ = static_cast<size_t>(memory_buffer_int32[2]); > > But again, I think having a struct for the header would be better. Using a struct now. Not sure if you wanted it only here or throughout the class. I did the latter. I'm also using sizeof on it to determine the header size, instead of a constant. Then, handling padding/packing/alignment shouldn't be needed. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.cc:147: int32* memory_buffer_int32 = reinterpret_cast<int32*>(memory_buffer_); On 2013/04/02 19:34:08, tommi wrote: > same here and below Done. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... File content/common/circular_memory_buffer.h (right): https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:26: class CircularMemoryBuffer { On 2013/04/02 19:34:08, tommi wrote: > Can we reflect the non-wrapping part better in the name? CircularMemoryBuffer > suggests to me that it's a regular circular buffer. Having said that, nothing > good comes to mind... PartialCircularBuffer? Yes that sounds good. PartialCircularBuffer is the best i can think of too... Done. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:31: void InitRead(); On 2013/04/02 19:34:08, tommi wrote: > Could we have two separate constructors instead of init functions? I imagine > that the InitRead() would take a const uint8* whereas the write constructor > would accept uint8*. Yes, that sounds good. Done. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:45: int8* memory_buffer_; On 2013/04/02 19:34:08, tommi wrote: > nit: I think we tend to use uint8 for raw bytes of data. Done. https://codereview.chromium.org/13473005/diff/1/content/common/circular_memor... content/common/circular_memory_buffer.h:52: size_t total_written_; On 2013/04/02 19:34:08, tommi wrote: > is total_written_ used for reading? Yes, it stores the header info how much was written to the buffer so that we don't read out of that bound.
https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... File content/common/circular_memory_buffer.cc (right): https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.cc:9: #define MIN3(a, b, c) std::min(a, std::min(b, c)) in the spirit of include-what-you-use I think you need #include <algorithm> here. https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.cc:34: void PartialCircularBuffer::InitRead() { Why do we need InitRead and InitWrite? Can we do this in the relevant constructors? https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.cc:173: void PartialCircularBuffer::UpdateAndWriteHeaderData(size_t added_length, instead of having ReadHeaderData, WriteHeaderData and UpdateAndWriteHeaderData, what about making the struct like so: struct BufferData { uint32 total_written; uint32 wrap_position; uint32 end_position; uint8 data[1]; }; Then use the data member to access the data and update all the fields directly instead of via memcpy. You'd change header_data_ to be a pointer, get rid of the memory_buffer_read_ and memory_buffer_write_ variables, |position_| would always be a zero based index into BufferData::data and there's no need to do +sizeof(header) calculations. https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... File content/common/circular_memory_buffer.h (right): https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.h:19: // circular. The first 3 * 4 bytes (int32) = 12 bytes + possible padding is the see comment about this below. As is sizeof(header) on 64bit platforms would be 3*8==24 bytes. On 64bit platforms using uint32 instead of size_t it would be 16 bytes if you don't specify how the packing should be done. https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.h:42: struct HeaderData { I don't think it's a good idea to use size_t here since size_t isn't always the same size. Before you were using uint32 and I think that's appropriate here. I also think that we should force packing to be 4 (sizeof(uint32)) here as well. If we don't do that then the size of the struct on 64bit platforms cold be 4*sizeof(uint32) and not 3*sizeof(uint32). See #pragma pack elsewhere in the code for how to do this. https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.h:58: HeaderData header_data_; maybe you should keep this variable last in the class to make life easier for the compiler (and for us to read the crash dumps). That way padding will be added to the end of the class instead of between this variable and memory_buffer_read_ .
Code review fixes including changed to use pointer to struct. I suggest you read the last comment in the .cc file first and the rest will be clearer. https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... File content/common/circular_memory_buffer.cc (right): https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.cc:9: #define MIN3(a, b, c) std::min(a, std::min(b, c)) On 2013/04/03 14:11:32, tommi wrote: > in the spirit of include-what-you-use I think you need #include <algorithm> > here. Done. Forgot to run lint before uploading. :-) https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.cc:34: void PartialCircularBuffer::InitRead() { On 2013/04/03 14:11:32, tommi wrote: > Why do we need InitRead and InitWrite? Can we do this in the relevant > constructors? Not really. Moved to ctors. https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.cc:173: void PartialCircularBuffer::UpdateAndWriteHeaderData(size_t added_length, On 2013/04/03 14:11:32, tommi wrote: > instead of having ReadHeaderData, WriteHeaderData and UpdateAndWriteHeaderData, > what about making the struct like so: > > struct BufferData { > uint32 total_written; > uint32 wrap_position; > uint32 end_position; > uint8 data[1]; > }; > > Then use the data member to access the data and update all the fields directly > instead of via memcpy. Indeed. Changed. > You'd change header_data_ to be a pointer, get rid of the memory_buffer_read_ > and memory_buffer_write_ variables, |position_| would always be a zero based > index into BufferData::data and there's no need to do +sizeof(header) > calculations. Changed header_data_ to buffer_data_. If you still want the buffer ptr argument to the read ctor should be const, it would need to come in two versions. For now I removed const. Knowing the header size is still necessary to be able to check how much is left of the mem buffer and not write oob. Also for some dchecks. I'm thinking if doing: data_size_ = memory_buffer_size_ - (buffer_data_->data - reinterpret_cast<uint8*>(buffer_data_)); and use data_size_ throughout the code. Then setting the packing shouldn't be necessary. And it shouldn't really matter how large the header actually is. Or do you think it's better to set the packing and use a constant? https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... File content/common/circular_memory_buffer.h (right): https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.h:19: // circular. The first 3 * 4 bytes (int32) = 12 bytes + possible padding is the On 2013/04/03 14:11:32, tommi wrote: > see comment about this below. As is sizeof(header) on 64bit platforms would be > 3*8==24 bytes. On 64bit platforms using uint32 instead of size_t it would be 16 > bytes if you don't specify how the packing should be done. Absolutely, I had forgot to update this after those changes. I've however now changed to the struct pointer approach (without packing; see other comment) and then I think it be better not to discuss hard numbers here at all. The point is that there's a small header, and the buffer size must be larger than that. For normal cases the buffer will probably be much larger, since the purpose of the class is to deal with pretty large amounts of data. What do you think? https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.h:42: struct HeaderData { On 2013/04/03 14:11:32, tommi wrote: > I don't think it's a good idea to use size_t here since size_t isn't always the > same size. Before you were using uint32 and I think that's appropriate here. > > I also think that we should force packing to be 4 (sizeof(uint32)) here as well. > If we don't do that then the size of the struct on 64bit platforms cold be > 4*sizeof(uint32) and not 3*sizeof(uint32). > > See #pragma pack elsewhere in the code for how to do this. Agree. Changed to uint32. I can still set the packing if you think, but not sure if needed. See my other comment. Not sure either how much the penalty would be to pack it, i.e. if there's a good reason not to pack (given that other potential problems are avoided which I think they are). https://codereview.chromium.org/13473005/diff/5001/content/common/circular_me... content/common/circular_memory_buffer.h:58: HeaderData header_data_; On 2013/04/03 14:11:32, tommi wrote: > maybe you should keep this variable last in the class to make life easier for > the compiler (and for us to read the crash dumps). That way padding will be > added to the end of the class instead of between this variable and > memory_buffer_read_ . It's now a pointer.
thanks for making all the changes. Sorry I didn't mention this before but I think we need a unit test to go with this cl. https://codereview.chromium.org/13473005/diff/18001/content/common/partial_ci... File content/common/partial_circular_buffer.cc (right): https://codereview.chromium.org/13473005/diff/18001/content/common/partial_ci... content/common/partial_circular_buffer.cc:59: uint8* buffer_uint8 = static_cast<uint8*>(buffer); use reinterpret_cast in these cases. void* and uint8* aren't related.
I'm happy to let tommi be the primary reviewer here. I agree that this seems like a perfect opportunity for very thorough unit testing.
Added unit test. Code review fix. Fixed some errors. Fixed dchecks. Change in content/content_common.gypi is only due to rebase. https://chromiumcodereview.appspot.com/13473005/diff/18001/content/common/par... File content/common/partial_circular_buffer.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/18001/content/common/par... content/common/partial_circular_buffer.cc:59: uint8* buffer_uint8 = static_cast<uint8*>(buffer); On 2013/04/05 14:43:07, tommi wrote: > use reinterpret_cast in these cases. void* and uint8* aren't related. Done.
https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... File content/common/partial_circular_buffer_unittest.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:21: const uint8 kInputData[14] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}; nit: you can leave the size of the buffer unspecified and let the compiler figure it out. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:51: size_t input_data_size_; I don't think you need these two member variables (input_data_size_ and buffer_size_). Instead you can just use sizeof(kInputData) or define constants where kInputData is defined. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:56: TEST_F(PartialCircularBufferTest, TestHeaderSize) { do we need a test for this? Can we instead have a compile time assert on the size? https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:65: uint8 output_data[14]; instead use sizeof(kInputData) and initialize the buffer to all zeros in one step: uint8 output_data[sizeof(kInputData)] = {0}; and lines 66-68 aren't needed https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:71: EXPECT_EQ(0, memcmp(kInputData, output_data, input_data_size_)); Is there a way to determine if more can be read? I think it would be a good idea to check that no more can be read at this point. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:79: uint8 output_data[28]; same here as for output_data above. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:84: const uint8 output_ref_data[28] = same comments for specifying the size of the array. Try to define these sizes only once and use sizeof etc elsewhere so that the size is guaranteed to be the same everywhere and the code is more easily maintainable. (e.g. if someone changes the array, all places that depend on the array shouldn't need to be manually updated).
Code review fixes. Removed packing of struct - checking header size in test instead and allocating buffer based on that size. Also did some minor refactoring. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... File content/common/partial_circular_buffer_unittest.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:21: const uint8 kInputData[14] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}; On 2013/04/11 03:09:43, tommi wrote: > nit: you can leave the size of the buffer unspecified and let the compiler > figure it out. Done. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:51: size_t input_data_size_; On 2013/04/11 03:09:43, tommi wrote: > I don't think you need these two member variables (input_data_size_ and > buffer_size_). Instead you can just use sizeof(kInputData) or define constants > where kInputData is defined. Done. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:56: TEST_F(PartialCircularBufferTest, TestHeaderSize) { On 2013/04/11 03:09:43, tommi wrote: > do we need a test for this? Can we instead have a compile time assert on the > size? Actually, I think it's better to not to rely on a header size assumption. That way the struct doesn't have to be packed either. I changed it so that it specifies the data size as a constant and checks the header size run time and then allocates the correct total size. Removed this test. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:65: uint8 output_data[14]; On 2013/04/11 03:09:43, tommi wrote: > instead use sizeof(kInputData) and initialize the buffer to all zeros in one > step: > uint8 output_data[sizeof(kInputData)] = {0}; > and lines 66-68 aren't needed Done. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:71: EXPECT_EQ(0, memcmp(kInputData, output_data, input_data_size_)); On 2013/04/11 03:09:43, tommi wrote: > Is there a way to determine if more can be read? I think it would be a good > idea to check that no more can be read at this point. Good point. Yes it returns 0 if no more can be read. Added test for this. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:79: uint8 output_data[28]; On 2013/04/11 03:09:43, tommi wrote: > same here as for output_data above. Done. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:84: const uint8 output_ref_data[28] = On 2013/04/11 03:09:43, tommi wrote: > same comments for specifying the size of the array. Try to define these sizes > only once and use sizeof etc elsewhere so that the size is guaranteed to be the > same everywhere and the code is more easily maintainable. (e.g. if someone > changes the array, all places that depend on the array shouldn't need to be > manually updated). Done throughout the code.
lgtm https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... File content/common/partial_circular_buffer.h (right): https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer.h:47: #pragma pack(push) I still think we should have the packing in there. For anything IPC related we use fixed sizes that are equal across all platforms. https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... File content/common/partial_circular_buffer_unittest.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer_unittest.cc:56: TEST_F(PartialCircularBufferTest, TestHeaderSize) { On 2013/04/11 09:24:18, Henrik Grunell wrote: > On 2013/04/11 03:09:43, tommi wrote: > > do we need a test for this? Can we instead have a compile time assert on the > > size? > > Actually, I think it's better to not to rely on a header size assumption. That > way the struct doesn't have to be packed either. I changed it so that it > specifies the data size as a constant and checks the header size run time and > then allocates the correct total size. Removed this test. Agreed https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... File content/common/partial_circular_buffer_unittest.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:21: const size_t kBufferDataSize = 52; should this be sizeof(kOutputRefDataWrap)? https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:22: const size_t kWrapPosition = 20; a comment here would be nice (I'm assuming 20 is just an arbitrary number, but it would be good to state that explicitly). https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:37: test_struct.data - reinterpret_cast<uint8*>(&test_struct); absolute nit: &test_struct.data[0] https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:59: scoped_array<uint8> buffer_; I think we now use scoped_ptr<uint8[]> instead of scoped_array. You might want to double check that.
I'll fix the last minor comments from Tommi tomorrow. Brett; please look at this now.
Code review fixes. content/content_tests.gypi is rebase only. https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... File content/common/partial_circular_buffer_unittest.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:21: const size_t kBufferDataSize = 52; On 2013/04/11 16:37:40, tommi wrote: > should this be sizeof(kOutputRefDataWrap)? That could be better. Done. https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:22: const size_t kWrapPosition = 20; On 2013/04/11 16:37:40, tommi wrote: > a comment here would be nice (I'm assuming 20 is just an arbitrary number, but > it would be good to state that explicitly). Correct. Added comment. https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:37: test_struct.data - reinterpret_cast<uint8*>(&test_struct); On 2013/04/11 16:37:40, tommi wrote: > absolute nit: &test_struct.data[0] Done. https://chromiumcodereview.appspot.com/13473005/diff/45001/content/common/par... content/common/partial_circular_buffer_unittest.cc:59: scoped_array<uint8> buffer_; On 2013/04/11 16:37:40, tommi wrote: > I think we now use scoped_ptr<uint8[]> instead of scoped_array. You might want > to double check that. Correct, it's deprecated. Done.
https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... File content/common/partial_circular_buffer.h (right): https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/par... content/common/partial_circular_buffer.h:47: #pragma pack(push) On 2013/04/11 16:37:39, tommi wrote: > I still think we should have the packing in there. For anything IPC related we > use fixed sizes that are equal across all platforms. Does this fall in the "IPC related" bin? It's an interpretation of the buffer contents that will take place in two places. The actual data isn't sent over IPC. And don't we get an unnecessary performance penalty?
Yes, shared memory is ipc. It's better to keep it explicit than having to think about platform bitness imho. I don't think there will be a measurable performance penalty. You can check what is done elsewhere and follow that but in general ipc messages and shared memory are part of the same package and should therefore follow the same rules. Sent from my phone. On Apr 14, 2013 7:06 AM, <grunell@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/13473005/diff/** > 29001/content/common/partial_**circular_buffer.h<https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/partial_circular_buffer.h> > File content/common/partial_**circular_buffer.h (right): > > https://chromiumcodereview.**appspot.com/13473005/diff/** > 29001/content/common/partial_**circular_buffer.h#newcode47<https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/partial_circular_buffer.h#newcode47> > content/common/partial_**circular_buffer.h:47: #pragma pack(push) > On 2013/04/11 16:37:39, tommi wrote: > >> I still think we should have the packing in there. For anything IPC >> > related we > >> use fixed sizes that are equal across all platforms. >> > > Does this fall in the "IPC related" bin? It's an interpretation of the > buffer contents that will take place in two places. The actual data > isn't sent over IPC. And don't we get an unnecessary performance > penalty? > > https://chromiumcodereview.**appspot.com/13473005/<https://chromiumcodereview... >
On 2013/04/14 16:06:31, tommi wrote: > Yes, shared memory is ipc. It's better to keep it explicit than having to > think about platform bitness imho. I don't think there will be a > measurable performance penalty. > You can check what is done elsewhere and follow that but in general ipc > messages and shared memory are part of the same package and should > therefore follow the same rules. > OK, good to know. Re-added the packing. gypi file changes are rebase only. Brett: please review now. > Sent from my phone. > On Apr 14, 2013 7:06 AM, <mailto:grunell@chromium.org> wrote: > > > > > https://chromiumcodereview.**appspot.com/13473005/diff/** > > > 29001/content/common/partial_**circular_buffer.h<https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/partial_circular_buffer.h> > > File content/common/partial_**circular_buffer.h (right): > > > > https://chromiumcodereview.**appspot.com/13473005/diff/** > > > 29001/content/common/partial_**circular_buffer.h#newcode47<https://chromiumcodereview.appspot.com/13473005/diff/29001/content/common/partial_circular_buffer.h#newcode47> > > content/common/partial_**circular_buffer.h:47: #pragma pack(push) > > On 2013/04/11 16:37:39, tommi wrote: > > > >> I still think we should have the packing in there. For anything IPC > >> > > related we > > > >> use fixed sizes that are equal across all platforms. > >> > > > > Does this fall in the "IPC related" bin? It's an interpretation of the > > buffer contents that will take place in two places. The actual data > > isn't sent over IPC. And don't we get an unnecessary performance > > penalty? > > > > > https://chromiumcodereview.**appspot.com/13473005/%3Chttps://chromiumcoderevi...> > >
LGTM based on tommi's review (didn't look at the details, just marked something random I noticed). https://chromiumcodereview.appspot.com/13473005/diff/68001/content/common/par... File content/common/partial_circular_buffer.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/68001/content/common/par... content/common/partial_circular_buffer.cc:11: #define MIN3(a, b, c) std::min(a, std::min(b, c)) Can this be an inline function? It looks like you only even need a uint32 version.
LGTM based on tommi's review (didn't look at the details, just marked something random I noticed). https://chromiumcodereview.appspot.com/13473005/diff/68001/content/common/par... File content/common/partial_circular_buffer.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/68001/content/common/par... content/common/partial_circular_buffer.cc:11: #define MIN3(a, b, c) std::min(a, std::min(b, c)) Can this be an inline function? It looks like you only even need a uint32 version.
lgtm % one request https://chromiumcodereview.appspot.com/13473005/diff/77001/content/common/par... File content/common/partial_circular_buffer.h (right): https://chromiumcodereview.appspot.com/13473005/diff/77001/content/common/par... content/common/partial_circular_buffer.h:59: uint32 Min3(uint32 a, uint32 b, uint32 c) { I don't think this needs to be a member function. Just make it an inlinable function in the cc file. (and move the algorithm include as well)
https://chromiumcodereview.appspot.com/13473005/diff/68001/content/common/par... File content/common/partial_circular_buffer.cc (right): https://chromiumcodereview.appspot.com/13473005/diff/68001/content/common/par... content/common/partial_circular_buffer.cc:11: #define MIN3(a, b, c) std::min(a, std::min(b, c)) On 2013/04/17 17:48:12, brettw wrote: > Can this be an inline function? It looks like you only even need a uint32 > version. Yes, that's better. Done. https://chromiumcodereview.appspot.com/13473005/diff/77001/content/common/par... File content/common/partial_circular_buffer.h (right): https://chromiumcodereview.appspot.com/13473005/diff/77001/content/common/par... content/common/partial_circular_buffer.h:59: uint32 Min3(uint32 a, uint32 b, uint32 c) { On 2013/04/18 16:38:52, tommi wrote: > I don't think this needs to be a member function. Just make it an inlinable > function in the cc file. (and move the algorithm include as well) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/13473005/92001
Message was sent while issue was closed.
Change committed as 195137 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
