Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16837003:
Disk cache: Update Addr to handle file format version 3. (Closed)
|
Created:
7 years, 6 months ago by rvargas (doing something else) Modified:
7 years, 6 months ago Reviewers:
gavinp CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionDisk cache: Update Addr to handle file format version 3.
Basically three new file block types appear: One for normal
entries, one for deleted entries and one for keeping track
of external files.
BUG=241277
TEST=net_unittests
R=gavinp@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207870
Patch Set 1 #
Total comments: 13
Patch Set 2 : #Patch Set 3 : #
Total comments: 21
Patch Set 4 : #Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h#newcode62 net/disk_cache/addr.h:62: value_ = ((file_type << kFileTypeOffset) & kFileTypeMask) | While we're here, what do you think of this idea: union NewCacheAddr { struct { uint32 : 28; uint32 file_type : 3; uint32 is_initialized : 1; } common; struct { uint32 file_number : 28; } separate_file; struct { uint32 start_block : 16; uint32 file_selector : 8; uint32 block_count : 2; uint32 : 2; } blockfile; }; I think this would really make the rest of the methods in this class a lot more readable.
https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h#newcode62 net/disk_cache/addr.h:62: value_ = ((file_type << kFileTypeOffset) & kFileTypeMask) | On 2013/06/12 21:06:50, gavinp wrote: > While we're here, what do you think of this idea: > > union NewCacheAddr { > struct { > uint32 : 28; > uint32 file_type : 3; > uint32 is_initialized : 1; > } common; > > struct { > uint32 file_number : 28; > } separate_file; > > struct { > uint32 start_block : 16; > uint32 file_selector : 8; > uint32 block_count : 2; > uint32 : 2; > } blockfile; > }; > > I think this would really make the rest of the methods in this class a lot more > readable. I hope you don't mind, but I don't like the union part and I would prefer not going to a struct with a bitfield... first, I don't want to rewrite this class; second, one objective of this class was for it to be a wrapper around a single int32, and while the struct and bitfield don't really change that, they obscure that fact too much.
On 2013/06/12 21:25:27, rvargas wrote: > https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h > File net/disk_cache/addr.h (right): > > https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h#newcode62 > net/disk_cache/addr.h:62: value_ = ((file_type << kFileTypeOffset) & > kFileTypeMask) | > On 2013/06/12 21:06:50, gavinp wrote: > > While we're here, what do you think of this idea: > > > > union NewCacheAddr { > > struct { > > uint32 : 28; > > uint32 file_type : 3; > > uint32 is_initialized : 1; > > } common; > > > > struct { > > uint32 file_number : 28; > > } separate_file; > > > > struct { > > uint32 start_block : 16; > > uint32 file_selector : 8; > > uint32 block_count : 2; > > uint32 : 2; > > } blockfile; > > }; > > > > I think this would really make the rest of the methods in this class a lot > more > > readable. > > I hope you don't mind, but I don't like the union part and I would prefer not > going to a struct with a bitfield... first, I don't want to rewrite this class; > second, one objective of this class was for it to be a wrapper around a single > int32, and while the struct and bitfield don't really change that, they obscure > that fact too much. I don't mind at all; I just wanted to make sure the idea was out there to talk about. You're going to be maintaining this code most of the time, so I'll defer to your judgement. Your arguments are fair ones, so I'll continue with the review. I'm particularly sensitive to the first argument about not rewriting the class. On the second point, I'm not sure who the audience that benefits from the explicitness is. As a reviewer, I have to slowly and carefully check all the masking operations are correct, particularly many of the methods that use vulgar constants like "8" to set the is_initialized flag. The bitfields would leave me trusting the compiler to get this right. Anybody wishing to understand the disk format would see the union and quickly know what's going on, particularly if we added a first element: "uint32 raw_value;" I'll continue with the review now, thanks for your patience and speed in answering my question.
I can't really review this; the knowledge of the format of a CacheAddr is scattered across too many places, and there's too many magic numbers to cross check over to other files. https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc#newcode59 net/disk_cache/addr.cc:59: if (((value_ & kFileTypeMask) >> kFileTypeOffset) > BLOCK_FILES) I don't understand this. https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc#newcode65 net/disk_cache/addr.cc:65: const uint32 kReservedBitsMask = 0x0c000000; What's 0x0c000000? https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h#newcode117 net/disk_cache/addr.h:117: return Addr(((8 + BLOCK_ENTRIES) << kFileTypeOffset) + value); 8? https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h#newcode137 net/disk_cache/addr.h:137: return 104; These magic numbers are killing me. Why 104?
I think the change that would help me here is to get a bit of religion about the setters/getters. Can we avoid referencing value_ in anything other than a hacker_style_inline_getter_or_setter?
On 2013/06/13 13:10:58, gavinp wrote: > I can't really review this; the knowledge of the format of a CacheAddr is > scattered across too many places, and there's too many magic numbers to cross > check over to other files. I'm actually trying to move any code that "understands" the format of a cache address to addr.* so if you see something out of there, let me know. Now, inside addr.*, yes, the code is quite aware of the magic needed to interpret the format. Please take another look.
(I hate that replying doesn't send the comments) https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc#newcode57 net/disk_cache/addr.cc:57: return !value_; Just so that I understand your comment, are you saying that using value_ here is confusing? (and replacing this with "return !value()" is better? https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc#newcode59 net/disk_cache/addr.cc:59: if (((value_ & kFileTypeMask) >> kFileTypeOffset) > BLOCK_FILES) On 2013/06/13 13:10:58, gavinp wrote: > I don't understand this. This is exactly the same check of SanityCheckV2 that verifies that the number stored on the file type makes sense. In this case, an address can not point to a file that stores entries (active or deleted) because that would mean the value we're verifying is the address of an entry (ie, SanityCheckForEntryV3). But yes, there is no reason for this code not to use file_type(), it was a sloppy copy paste from v2. https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc#newcode65 net/disk_cache/addr.cc:65: const uint32 kReservedBitsMask = 0x0c000000; On 2013/06/13 13:10:58, gavinp wrote: > What's 0x0c000000? That's the mask for the reserved bits of the value, as described on the header under "reserved bits" (line 53). I'm just following the rule of defining constants as close to the point of use as possible. I moved the definition to an anonymous namespace. https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h#newcode117 net/disk_cache/addr.h:117: return Addr(((8 + BLOCK_ENTRIES) << kFileTypeOffset) + value); On 2013/06/13 13:10:58, gavinp wrote: > 8? Rewrote it on a more explicit way. https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.h#newcode137 net/disk_cache/addr.h:137: return 104; On 2013/06/13 13:10:58, gavinp wrote: > These magic numbers are killing me. Why 104? Because there is a COMPILE_ASSERT(sizeof(EntryRecord) == 104) at line 172 of the file format definition, and BLOCK_ENTRIES is the file that stores regular entries (aka, entry records). Sure, this could be sizeof() (at the cost of a cast), but that means that this file would have to include v3/disk_format_.h, but it only includes disk_format_base because it is used by both v2 and v3, and there is no way to include both definitions on a single compilation unit.
Much easier to read. Thanks for your patience in cycling this. https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc#newcode57 net/disk_cache/addr.cc:57: return !value_; On 2013/06/13 19:07:25, rvargas wrote: > Just so that I understand your comment, are you saying that using value_ here is > confusing? (and replacing this with "return !value()" is better? No, and this is probably one of the few uses of value that wouldn't bug me. It'd be good to comment: // If it's uninitialized, all fields should be zero. Just so we know that we are doing something evil and wrong by using value_. https://codereview.chromium.org/16837003/diff/1/net/disk_cache/addr.cc#newcode65 net/disk_cache/addr.cc:65: const uint32 kReservedBitsMask = 0x0c000000; On 2013/06/13 19:07:25, rvargas wrote: > On 2013/06/13 13:10:58, gavinp wrote: > > What's 0x0c000000? > > That's the mask for the reserved bits of the value, as described on the header > under "reserved bits" (line 53). I'm just following the rule of defining > constants as close to the point of use as possible. > > I moved the definition to an anonymous namespace. Yeah, I saw that. But why not reserved_bits() == 0? https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:41: CacheAddr new_value = value() & ~kFileTypeMask; Addr ret(*this); ret.set_file_type(EXTERNAL); ?? What is AsExternal returning? Why was this addr wrong? https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:47: CacheAddr new_value = value() + (BLOCK_FILES << kFileTypeOffset); Addr ret(*this); ret.set_file_type(BLOCK_FILES); ?? What is AsBlockFile returning? Why was this addr wrong? https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:117: return Addr(kInitializedMask + (BLOCK_ENTRIES << kFileTypeOffset) + value); Can value here have a block count & file selector, or is it only a block#? https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:171: static const uint32 kFileTypeOffset = 28; Here's another good spot for vertical whitespace. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:176: static const uint32 kStartBlockMask = 0x0000FFFF; Probably a good idea to put vertical whitespace here.
How about now? https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:41: CacheAddr new_value = value() & ~kFileTypeMask; On 2013/06/13 19:49:00, gavinp wrote: > Addr ret(*this); > ret.set_file_type(EXTERNAL); > > ?? > > What is AsExternal returning? Why was this addr wrong? BLOCK_FILES is a type of file that holds a small marker about an external file. So... I can have an entry with a stream pointing to 0x10040005 (f_040005), and that also means that the fifth block of block_file 4 contains the marker for this file. So the address for the marker would be 0xd0040005. These methods translate between the two addresses. I understand that the mechanism has to be explained somewhere, but I don't think this file is the right place to do that. With that piece of knowledge, the description on the header looks clear enough (in fact, it seems clear enough for someone randomly going through the header without having the full picture of how things work). https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:47: CacheAddr new_value = value() + (BLOCK_FILES << kFileTypeOffset); On 2013/06/13 19:49:00, gavinp wrote: > Addr ret(*this); > ret.set_file_type(BLOCK_FILES); The only thing we need is BLOCK_FILES (5) -> 0, and 0 -> 5. Doing 0 << n to write a 0 somewhere bugs me (yes I know, in the big picture it doesn't matter). On the other hand, a generic method to set the type has to deal with xx -> yy, so it first has to clear, with the same pattern that I'm using, and then set, with the second pattern... all the time. So we still have the same two lines you don't like (now one after the other), more function calls (and more code), and we do more work all the time. but sure, slightly easier to read. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:117: return Addr(kInitializedMask + (BLOCK_ENTRIES << kFileTypeOffset) + value); On 2013/06/13 19:49:00, gavinp wrote: > Can value here have a block count & file selector, or is it only a block#? The number of blocks is fixed to 1 (0 on the bit pattern), the file number is anything. In other words, value is up to 22 bits (but that is verified by the caller when reading values from the index)
https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:41: CacheAddr new_value = value() & ~kFileTypeMask; On 2013/06/13 22:46:29, rvargas wrote: > On 2013/06/13 19:49:00, gavinp wrote: > > Addr ret(*this); > > ret.set_file_type(EXTERNAL); > > > > ?? > > > > What is AsExternal returning? Why was this addr wrong? > > BLOCK_FILES is a type of file that holds a small marker about an external file. > > So... I can have an entry with a stream pointing to 0x10040005 (f_040005), and > that also means that the fifth block of block_file 4 contains the marker for > this file. So the address for the marker would be 0xd0040005. What's a marker for a file? > These methods translate between the two addresses. > > I understand that the mechanism has to be explained somewhere, but I don't think > this file is the right place to do that. With that piece of knowledge, the > description on the header looks clear enough (in fact, it seems clear enough for > someone randomly going through the header without having the full picture of how > things work). https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:171: static const uint32 kFileTypeOffset = 28; On 2013/06/13 19:49:00, gavinp wrote: > Here's another good spot for vertical whitespace. These constants are difficult to understand. They are redundant but there are no assertions on that. They refer to bifurcated views of the memory (aka: a union), but there's no documentation of that here (although there's a comment ~150 lines up). Can I get some blank line love, please? With some separation, a future reader will have some suspicion that there's a union here. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr_unitt... File net/disk_cache/addr_unittest.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr_unitt... net/disk_cache/addr_unittest.cc:40: EXPECT_TRUE(Addr(0x80001000).SanityCheckV2()); Can we use the multiargument constructor to Addr throughout this test? We'll want add a second one for file_type == EXTERNAL, I guess.
https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:41: CacheAddr new_value = value() & ~kFileTypeMask; On 2013/06/13 23:38:27, gavinp wrote: > On 2013/06/13 22:46:29, rvargas wrote: > > On 2013/06/13 19:49:00, gavinp wrote: > > > Addr ret(*this); > > > ret.set_file_type(EXTERNAL); > > > > > > ?? > > > > > > What is AsExternal returning? Why was this addr wrong? > > > > BLOCK_FILES is a type of file that holds a small marker about an external > file. > > > > So... I can have an entry with a stream pointing to 0x10040005 (f_040005), and > > that also means that the fifth block of block_file 4 contains the marker for > > this file. So the address for the marker would be 0xd0040005. > > What's a marker for a file? The hash of the key (although I'm saving also EntryRecord.pad1 but I doubt the hash will be ever increased to 64 bits). https://docs.google.com/a/google.com/document/d/1UnBlsdJusO4pdgzC4Ut194YVki16... > > > These methods translate between the two addresses. > > > > I understand that the mechanism has to be explained somewhere, but I don't > think > > this file is the right place to do that. With that piece of knowledge, the > > description on the header looks clear enough (in fact, it seems clear enough > for > > someone randomly going through the header without having the full picture of > how > > things work). > > https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:171: static const uint32 kFileTypeOffset = 28; On 2013/06/13 23:38:27, gavinp wrote: > On 2013/06/13 19:49:00, gavinp wrote: > > Here's another good spot for vertical whitespace. > > These constants are difficult to understand. They are redundant but there are no > assertions on that. They refer to bifurcated views of the memory (aka: a union), > but there's no documentation of that here (although there's a comment ~150 lines > up). > > Can I get some blank line love, please? With some separation, a future reader > will have some suspicion that there's a union here. > I'm sorry, but I read your comment and I couldn't figure out what kind of grouping were you looking for. This doesn't trigger any of my internal rules of when to add vertical spacing for readability. That means that I have no problem reading them or understanding what they mean :(. They are not meant to show any split personality problem for Addr, they just go through the documented format and define a mask and offset (number of bits to shift) for every component of the address. I find that to be an extremely familiar language. So adding the two empty lines that you suggested looks to me like two random lines (literally, I'm not making this up). The only explanation I could come up was to get better alignment of which value belongs to which name, but that was just too hard to believe given that each line looks different from the next one. Redundant in that you can derive 0x70000000 from 28? That's the point but I don't think that deserves an assertion. I see your point of the documentation being 150 lines up, but the documentation is the explanation of what an address is so it has to be at the top of the file, and the constants are an implementation detail that should stay on the private part, here, so I don't know how to make that simpler without duplicating the comment, and I don't think that's something sane to do. I'm honestly baffled hearing that masks are hard to understand :( So it's not lack of love. It's that I would most likely ask for empty lines to be _removed_ if the review were going the other way around. And you would probably ignore me because it sits on the field of personal opinion. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr_unitt... File net/disk_cache/addr_unittest.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr_unitt... net/disk_cache/addr_unittest.cc:40: EXPECT_TRUE(Addr(0x80001000).SanityCheckV2()); On 2013/06/13 23:38:27, gavinp wrote: > Can we use the multiargument constructor to Addr throughout this test? part of the purpose of this test is to go through actual bit patterns, not to go through how Addr can be built from the components. Other tests already do that. > > We'll want add a second one for file_type == EXTERNAL, I guess. what do you mean by a second one? Another EXPECT_TRUE line? What number would you like to try?
not lgtm. I can't review this code; it's too difficult to read and too diffuse. No amount of telling me it's readable will make me understand it. I can look at it again when: 1. value_ is only ever loaded or stored from getter and setter methods, possibly inline that shift and mask. 2. the constructor from int is used almost never. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:41: CacheAddr new_value = value() & ~kFileTypeMask; On 2013/06/14 02:15:44, rvargas wrote: > On 2013/06/13 23:38:27, gavinp wrote: > > On 2013/06/13 22:46:29, rvargas wrote: > > > On 2013/06/13 19:49:00, gavinp wrote: > > > > Addr ret(*this); > > > > ret.set_file_type(EXTERNAL); > > > > > > > > ?? > > > > > > > > What is AsExternal returning? Why was this addr wrong? > > > > > > BLOCK_FILES is a type of file that holds a small marker about an external > > file. > > > > > > So... I can have an entry with a stream pointing to 0x10040005 (f_040005), > and > > > that also means that the fifth block of block_file 4 contains the marker for > > > this file. So the address for the marker would be 0xd0040005. > > > > What's a marker for a file? > > The hash of the key (although I'm saving also EntryRecord.pad1 but I doubt the > hash will be ever increased to 64 bits). > I don't understand this at all. Please remove Addr::AsExternal() and Addr::AsBlockFile() and add them in a review in which they are not dead. > https://docs.google.com/a/google.com/document/d/1UnBlsdJusO4pdgzC4Ut194YVki16... > > > > > > > > These methods translate between the two addresses. > > > > > > I understand that the mechanism has to be explained somewhere, but I don't > > think > > > this file is the right place to do that. With that piece of knowledge, the > > > description on the header looks clear enough (in fact, it seems clear enough > > for > > > someone randomly going through the header without having the full picture of > > how > > > things work). > > > > > https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:47: CacheAddr new_value = value() + (BLOCK_FILES << kFileTypeOffset); On 2013/06/13 22:46:29, rvargas wrote: > On 2013/06/13 19:49:00, gavinp wrote: > > Addr ret(*this); > > ret.set_file_type(BLOCK_FILES); > > The only thing we need is BLOCK_FILES (5) -> 0, and 0 -> 5. > > Doing 0 << n to write a 0 somewhere bugs me (yes I know, in the big picture it > doesn't matter). On the other hand, a generic method to set the type has to deal > with xx -> yy, so it first has to clear, with the same pattern that I'm using, > and then set, with the second pattern... all the time. So we still have the same > two lines you don't like (now one after the other), more function calls (and > more code), and we do more work all the time. > > but sure, slightly easier to read. They compile to the same thing. See: #include <stdint.h> const uint32_t mask = 0x000ff000; const int shift = 12; /* 89 f8 mov %edi,%eax 25 ff 0f f0 ff and $0xfff00fff,%eax c3 retq */ uint32_t foo(uint32_t i) { return i & ~mask; } /* 89 f0 mov %esi,%eax 81 e7 ff 0f f0 ff and $0xfff00fff,%edi c1 e0 0c shl $0xc,%eax 09 f8 or %edi,%eax c3 retq */ uint32_t quux(uint32_t i, int v) { return (v << shift) | (i & ~mask); } /* 89 f8 mov %edi,%eax 25 ff 0f f0 ff and $0xfff00fff,%eax c3 retq */ uint32_t frob(uint32_t i) { return quux(i, 0); } https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:171: static const uint32 kFileTypeOffset = 28; On 2013/06/14 02:15:44, rvargas wrote: > On 2013/06/13 23:38:27, gavinp wrote: > > On 2013/06/13 19:49:00, gavinp wrote: > > > Here's another good spot for vertical whitespace. > > > > These constants are difficult to understand. They are redundant but there are > no > > assertions on that. They refer to bifurcated views of the memory (aka: a > union), > > but there's no documentation of that here (although there's a comment ~150 > lines > > up). > > > > Can I get some blank line love, please? With some separation, a future reader > > will have some suspicion that there's a union here. > > > > I'm sorry, but I read your comment and I couldn't figure out what kind of > grouping were you looking for. This doesn't trigger any of my internal rules of > when to add vertical spacing for readability. > > That means that I have no problem reading them or understanding what they mean > :(. They are not meant to show any split personality problem for Addr, they just > go through the documented format and define a mask and offset (number of bits to > shift) for every component of the address. I find that to be an extremely > familiar language. ... for every component of the address, except the reserved bits. > > So adding the two empty lines that you suggested looks to me like two random > lines (literally, I'm not making this up). The only explanation I could come up > was to get better alignment of which value belongs to which name, but that was > just too hard to believe given that each line looks different from the next one. > > Redundant in that you can derive 0x70000000 from 28? That's the point but I > don't think that deserves an assertion. > > I see your point of the documentation being 150 lines up, but the documentation > is the explanation of what an address is so it has to be at the top of the file, > and the constants are an implementation detail that should stay on the private > part, here, so I don't know how to make that simpler without duplicating the > comment, and I don't think that's something sane to do. > > I'm honestly baffled hearing that masks are hard to understand :( > > So it's not lack of love. It's that I would most likely ask for empty lines to > be _removed_ if the review were going the other way around. And you would > probably ignore me because it sits on the field of personal opinion.
Please don't nack a cl unless what you mean is that the feature should not land at all. If that is your intention, we seriously need to talk. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:41: CacheAddr new_value = value() & ~kFileTypeMask; > > > > > > What's a marker for a file? > > > > The hash of the key (although I'm saving also EntryRecord.pad1 but I doubt the > > hash will be ever increased to 64 bits). > > > > I don't understand this at all. Please remove Addr::AsExternal() and > Addr::AsBlockFile() and add them in a review in which they are not dead. > > > > https://docs.google.com/a/google.com/document/d/1UnBlsdJusO4pdgzC4Ut194YVki16... What is it that you don't understand? ---- Yes, I can remove the new methods (and in fact I'll upload a version doing that shortly), but I want to point out to you that having callers is _not_ a prerequisite to add a method when dealing with new functionality being added. (especially when there is a design document that mentions the specific functionality, and a public reference that show how the caller uses the new methods). I really thought two trivial methods like these would pose no problem for a review. I am wondering how am I going to be able to land the new code in a reasonable timeframe :( https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:47: CacheAddr new_value = value() + (BLOCK_FILES << kFileTypeOffset); You are aware that I changed the code to what you wanted a while ago, right? I fail to grasp what is your point. That a given compiler is able to inline a function call and optimize part of it? Where did I said that an optimizer would not be able to do that? Do you expect me to paste compiled code showing that there may be a function call (ergo not optimized part)? I can write hundreds of lines and show you that the compiler optimizes everything away, and that is not in itself a justification to write hundreds of lines... as I said on my comment, just because in reality it doesn't matter doesn't mean something is not going to bug me. And again, I did what you wanted a while ago. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:171: static const uint32 kFileTypeOffset = 28; > ... for every component of the address, except the reserved bits. And that would be because this is part of the implementation, not the definition of the format itself. And the only reason why all this definitions are on the header file at all is that they are used by the inlined code. Reserved bits are not used by any inlined function, that's why the needed constant is on the unnamed namespace on the cc file.
Yet another version up (with the methods that you asked removed). BTW, I'm not removing SanityCheckV3 and SanityCheckForEntryV3 even they are not called by anybody (more "dead code" to remove?)... in fact the whole CL could be "dead code" given that there is nothing exercising the V3 codepaths. I just realized that I didn't answer "2. the constructor from int is used almost never.". Could you point me to what do you want removed? I cannot answer a global request like that when I already pointed out why that constructor is used in the only place you expressed discomfort about it.
I want to help land the V3 cache as quickly as possible. I'm sorry if I can't devote as much time to that as I should; but I assure you it's a high priority of mine. I also want to insure that I can debug the cache. At present, the blockfile cache is very difficult for me to debug, because of how diffuse knowledge is. Understanding higher level constructs forces me to understand lower level constructs simultaneously in way that is painful. Changes like having Addr use setter/getter methods exclusively, which do not impact performance on any modern compiler and make numerous expressions testing address structure really help this goal. When later code lands using Addr, it'll be easier to understand too. Thank you for your patience. This code LGTM with the last access method and constants into the header. The tests can stay as they are. The SanityCheckV3 methods may be "dead", but I understand what they're doing, and there's enough context in this review for me to understand it. https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h File net/disk_cache/addr.h (right): https://codereview.chromium.org/16837003/diff/33001/net/disk_cache/addr.h#new... net/disk_cache/addr.h:171: static const uint32 kFileTypeOffset = 28; On 2013/06/14 19:25:17, rvargas wrote: > > ... for every component of the address, except the reserved bits. > > And that would be because this is part of the implementation, not the definition > of the format itself. And the only reason why all this definitions are on the > header file at all is that they are used by the inlined code. > > Reserved bits are not used by any inlined function, that's why the needed > constant is on the unnamed namespace on the cc file. Keeping the definitions all in one place is probably the higher value here. Add the constant and the inline definition. https://codereview.chromium.org/16837003/diff/51001/net/disk_cache/addr.cc File net/disk_cache/addr.cc (right): https://codereview.chromium.org/16837003/diff/51001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:11: bool AreReservedBitsValid(uint32 value) { Make this an inline getter. https://codereview.chromium.org/16837003/diff/51001/net/disk_cache/addr.cc#ne... net/disk_cache/addr.cc:13: const uint32 kReservedBitsMask = 0x0c000000; Move this into the header. https://codereview.chromium.org/16837003/diff/51001/net/disk_cache/addr_unitt... File net/disk_cache/addr_unittest.cc (right): https://codereview.chromium.org/16837003/diff/51001/net/disk_cache/addr_unitt... net/disk_cache/addr_unittest.cc:40: EXPECT_TRUE(Addr(0x80001000).SanityCheckV2()); These preexisting uses of the int constructor can stay. An alternative, that I'd to see for new tests of this type of thing, is to use a null constructor, a bunch of setter methods, followed by the sanity check. I appreciate that an assertion that the value() == 0x80001000 has a place too. I'm going to insist on this for new tests; I think we're missing a chance to improve this code while we're here, but I appreciate speed is important.
Message was sent while issue was closed.
Committed patchset #6 manually as r207870 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
