Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
394313002:
Add support for loading pak files from arbitrary file regions. (Closed)
|
Created:
6 years, 5 months ago by Primiano Tucci (use gerrit) Modified:
6 years, 4 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Torne, mkosiba (inactive) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd support for loading pak files from arbitrary file regions.
This is to support a new use case for Android: mmap a pak file
directly from the APK (where it will be stored uncompressed)
without extracting it first. This would save both precious space
on the flash and startup time on the first run.
This CL introduces:
- the necessary changes to base::File to memory map arbitrary
regions of a file.
- The corresponding changes (plus unittests) in DataPack and
ResourceBundle to take advantage of the new support.
At present state, this CL is not intended to introduce any
behavioral change.
BUG=394502
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285945
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Win implementation #
Total comments: 3
Patch Set 4 : Support arbitrary alignments #Patch Set 5 : Refactor + fix win #Patch Set 6 : Fix return value check + speculative fix on windows #Patch Set 7 : Add base unittest and fix win #
Total comments: 15
Patch Set 8 : Address first willchan@ comments #
Total comments: 9
Patch Set 9 : remove IsWholeFile, use kWholeFile; other nits #
Total comments: 4
Patch Set 10 : Ah, remove <algorithm> #
Total comments: 6
Patch Set 11 : Fix test + DCHECK on mask #Patch Set 12 : Move Region to MemoryMappedFile #
Total comments: 2
Messages
Total messages: 28 (0 generated)
Nico, are you a good OWNER for this or do you prefer me to ping somebody else? Thanks! https://codereview.chromium.org/394313002/diff/70001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/70001/base/files/file.h#newcod... base/files/file.h:163: explicit Region(File& file); Shame that this cannot be const. Unfortunately GetLength is deliberately non const. The alternative would be passing as const here and reinterpret_cast in the ctor, which sounds a bit odd. :-/ Or eventually just a naked pointer. I feel the non-cost reference is the cleanest option.
lgtm but I'm not an owner :) https://codereview.chromium.org/394313002/diff/70001/base/files/memory_mapped... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/394313002/diff/70001/base/files/memory_mapped... base/files/memory_mapped_file_win.cc:28: if (len <= 0 || len > kint32max) do we want to apply the same check to offset? then the check below would be guaranteed to not overflow.
https://codereview.chromium.org/394313002/diff/70001/base/files/memory_mapped... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/394313002/diff/70001/base/files/memory_mapped... base/files/memory_mapped_file_win.cc:28: if (len <= 0 || len > kint32max) On 2014/07/17 00:38:01, mkosiba wrote: > do we want to apply the same check to offset? then the check below would be > guaranteed to not overflow. Actually, now that I think more about this, we shouldn'd do any check at all here. mmap / MapViewOfFile is just supposed to handle this case and fail. There is no reason IMHO to replicate this behavior on our side.
willchan, tony, could you PTAL (respectively to base/ and ui/). Many thanks!
This may result in some unaligned reads from memory, but maybe that's OK? It sounds like on some architectures this might be slow (or maybe crash?), but maybe this code isn't that time sensitive. ui/base/resource LGTM
On 2014/07/17 22:55:26, tony wrote: > This may result in some unaligned reads from memory, but maybe that's OK? It > sounds like on some architectures this might be slow (or maybe crash?), but > maybe this code isn't that time sensitive. > > ui/base/resource LGTM Good point, I should have mentioned: on Android aapt-zipalign guarantees that the uncompressed files (which are the only that can be referenced from the APK) are aligned at at-least 4 bytes (can be increased in the cmdline). That should be fine for pakfiles (or otherwise would be dealth with in the build system to increase the alignment of zipalign)
I must apologize, I didn't get a chance to finish this review today. But I got started at least :) I'll finish it tomorrow. PS: Thanks for the test coverage. Very much appreciated. https://codereview.chromium.org/394313002/diff/260001/base/files/file.cc File base/files/file.cc (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.cc#newc... base/files/file.cc:27: bool File::Region::IsWholeFile() const { Hmmm...what happens when |size| == the length of the whole file, which is less than std::numeric_limits<int64>::max()? https://codereview.chromium.org/394313002/diff/260001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:159: struct BASE_EXPORT Region { Why is this a property of the file as opposed to the memory map? Unless there are other consumers that are interested in file regions (very possible! but I don't know of any yet), I'd prefer to keep this struct in MemoryMappedFile instead. https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:165: // Whether the region intends to cover the entire file. What's the intended behavior when a file is mapped using regions for the whole file, but afterward, the file is appended to. Should IsWholeFile() still return true? This should be documented and a test should cover this. https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file.cc:28: if (!MapFileToMemory(base::File::Region::WholeFile())) { No need for base::. https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file.h:68: bool MapFileToMemory(const base::File::Region& region); Perhaps rename this to MapFileRegionToMemory()
Many thanks for your quick response Will! Replies inline below, looking forward your next batch of comments. https://codereview.chromium.org/394313002/diff/260001/base/files/file.cc File base/files/file.cc (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.cc#newc... base/files/file.cc:27: bool File::Region::IsWholeFile() const { On 2014/07/21 23:55:40, willchan wrote: > Hmmm...what happens when |size| == the length of the whole file, which is less > than std::numeric_limits<int64>::max()? Good point, let me spend one minute to explain the rationale behind all this. If you think to this end-to-end, there are two ways you can map fully a file on Windows: MapViewOfFile(length=0, ...) and MVOF(length=file.size). Now, from a conceptual viewpoint the two variants should behave in the same way. On the other side, I'm an extremely paranoid person (especially when it comes to changes to base::) and I won't be surprised if some tiny corner case would hide behind this. In the very essence all this is a way to make it so that the existing code paths in chromium, which before this change did only MVOF(length=0, ...), keep passing length=0 at the end of their journey. In more concrete words: IsWholeFile() == true --> MVOF(length=0) # Make this change behave as a noop end-to-end IsWholeFile() == false --> MVOF(length=~region.length) Essentially, the way I intended IsWholeFile, is whether the caller expresses the desire of an old-style full-cover mmap/MVOF to preserve the same exact current behavior. https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file.cc:28: if (!MapFileToMemory(base::File::Region::WholeFile())) { On 2014/07/21 23:55:40, willchan wrote: > No need for base::. Done. https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/memory_mappe... base/files/memory_mapped_file.h:68: bool MapFileToMemory(const base::File::Region& region); On 2014/07/21 23:55:40, willchan wrote: > Perhaps rename this to MapFileRegionToMemory() Done.
There are more places I didn't mention that unnecessarily do base::. Other than that, this CL is pretty solid and close to done. https://codereview.chromium.org/394313002/diff/260001/base/files/file.cc File base/files/file.cc (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.cc#newc... base/files/file.cc:27: bool File::Region::IsWholeFile() const { On 2014/07/22 10:32:01, Primiano Tucci wrote: > On 2014/07/21 23:55:40, willchan wrote: > > Hmmm...what happens when |size| == the length of the whole file, which is less > > than std::numeric_limits<int64>::max()? > > Good point, let me spend one minute to explain the rationale behind all this. If > you think to this end-to-end, there are two ways you can map fully a file on > Windows: MapViewOfFile(length=0, ...) and MVOF(length=file.size). > Now, from a conceptual viewpoint the two variants should behave in the same way. > On the other side, I'm an extremely paranoid person (especially when it comes to > changes to base::) and I won't be surprised if some tiny corner case would hide > behind this. > > In the very essence all this is a way to make it so that the existing code paths > in chromium, which before this change did only MVOF(length=0, ...), keep passing > length=0 at the end of their journey. > > In more concrete words: > IsWholeFile() == true --> MVOF(length=0) # Make this change behave as a noop > end-to-end > IsWholeFile() == false --> MVOF(length=~region.length) > > Essentially, the way I intended IsWholeFile, is whether the caller expresses the > desire of an old-style full-cover mmap/MVOF to preserve the same exact current > behavior. We chatted about this offline. Primiano explained that the reason for doing this is to provide an API for mapping an entire file. We're going to try to achieve this by replacing the accessor with a constant, something like Region::kWholeFile. This will remove the confusion around the edge cases I described. https://codereview.chromium.org/394313002/diff/260001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:159: struct BASE_EXPORT Region { On 2014/07/21 23:55:40, willchan wrote: > Why is this a property of the file as opposed to the memory map? Unless there > are other consumers that are interested in file regions (very possible! but I > don't know of any yet), I'd prefer to keep this struct in MemoryMappedFile > instead. Can you address this comment? https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... base/files/memory_mapped_file.h:39: bool Initialize(File file, const base::File::Region& region); No need for base:: https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... base/files/memory_mapped_file.h:68: bool MapFileRegionToMemory(const base::File::Region& region); No need for base:: https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:12: using base::File; You don't need these using statements if everything's in the namespace, right? https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... File base/files/memory_mapped_file_win.cc (right): https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... base/files/memory_mapped_file_win.cc:5: #include <algorithm> This should go below memory_mapped_file.h, as per style guide. foo.h always goes first in foo.cc && foo_test.cc.
https://codereview.chromium.org/394313002/diff/280001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/280001/base/files/file.h#newco... base/files/file.h:160: static Region WholeFile(); Maybe-dumb drive-by suggestion: Maybe make this Region::AllOf(const File&) and use File::GetLength() to fill in the accurate size. Then the implementation of MemoryMappedFile wouldn't need to include its own size-determination.
Sorry for the delay, busy day. Will / Jeffrey, I hope I covered all the comments this time. (If not, I'll owe you one beer for every missed comment the next time you pass by London ;-) ) https://codereview.chromium.org/394313002/diff/260001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:159: struct BASE_EXPORT Region { On 2014/07/22 20:45:47, willchan wrote: > On 2014/07/21 23:55:40, willchan wrote: > > Why is this a property of the file as opposed to the memory map? Unless there > > are other consumers that are interested in file regions (very possible! but I > > don't know of any yet), I'd prefer to keep this struct in MemoryMappedFile > > instead. I'm on the fence on this. On your side: yes, the reason why I introduce the Region concept is essentially for mmap. On the other side: look at resource_bundle.cc (which is out of /base) in this CL (lines 238-242): It does something like this: void ResourceBundle::AddDataPackFromFile(base::File, ScaleFactor scale_factor) { AddDataPackFromFileRegion(file.Pass(), base::File::Region::kWholeFile, ...); ResourceBundle today already knows the concept of a File. ResourceBundle, however, is oblivious of the fact that DataPack (a class it uses) will do a mmap operation on that File (RB just passes the File along). If i keep the Region as a File-related concept, ResourceBundle will keep to be oblivious about the mmap implementation detail of DataPack. If I move Region to memory_mapped_file.h, now the ResourceBundle (and DataPack) method signatures will expose this further detail. Essentially the final signature of DataPack would become: AddDataPackFromFileRegion(base::File, MemoryMappedFile::Region), which IMHO is a bit ackward vs the current AddDataPackFromFileRegion(base::File, base::File::Region), which IMHO looks better. What do you think? https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:165: // Whether the region intends to cover the entire file. On 2014/07/21 23:55:40, willchan wrote: > What's the intended behavior when a file is mapped using regions for the whole > file, but afterward, the file is appended to. Should IsWholeFile() still return > true? This should be documented and a test should cover this. I think we discussed this offline and kWholeFile should remove any ambiguity. So...*puff* this method has gone. Together with it, also the empty ~dtor has disappeared. Rationale: even if it's empty, the static kWholeFile would have created an exit time destructor (or at least, clang's -Wexit-time-destructors strongly believed that, firing an error, which went away removing the dtor). https://codereview.chromium.org/394313002/diff/280001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/280001/base/files/file.h#newco... base/files/file.h:160: static Region WholeFile(); On 2014/07/22 21:22:29, Jeffrey Yasskin wrote: > Maybe-dumb drive-by suggestion: Maybe make this Region::AllOf(const File&) and > use File::GetLength() to fill in the accurate size. Then the implementation of > MemoryMappedFile wouldn't need to include its own size-determination. If you take a look to patchsets 3-4 I used that approach there, then switched to kWholeFile. Why? The reason is mostly this: I want to make sure that my change is a noop for the remaining existing chromium code (i.e. all the code paths that today do a mmap) w.r.t of the arguments passed to MapViewOfFile. In other words, if I do as you suggest, in mapped_file_win.cc, all the existing code paths would end-up calling MapViewOfFile(...size=ActualFileSize...). Instead, what happens today (for whole file mapping) is MapViewOfFile(...size=0...), where size=0 has the special semantic, in the WinAPI, of mapping the entire file. Now, from a logic viewpoint, providing the size ourselves or asking windows to calculate it for us *should* be equivalent. On the other side I don't have enough WinAPI expertise to be 100% sure that the two aforementioned patterns ( map(size=0) and map (size=actual_size)) will be always equivalent and the latter doesn't end up in some weird race/corner case I didn't consider (e.g., pseudo-files for which the stat.size == 0, but that the kernel can manage to mmap them without problems. Is that possible on Windows? What about symlinks?) . Curious side note. A method signature such as Foo(File, const FileRegion&) where FileRegion has a ctor as the one you suggest is curiously very prone to UAF by design, since File must be Pass()-ed. In other words, it is very tempting to use such a method signature like this: Foo(file.Pass(), FileRegion(file)), which would lead into a UAF of |file|, in the 2nd argument by the FileRegion ctor. :-) https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... base/files/memory_mapped_file.h:39: bool Initialize(File file, const base::File::Region& region); On 2014/07/22 20:45:47, willchan wrote: > No need for base:: Shame on me. sorry I should have rechecked all the rest when you pointed that out in your previous review. :-/ https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... base/files/memory_mapped_file.h:68: bool MapFileRegionToMemory(const base::File::Region& region); On 2014/07/22 20:45:47, willchan wrote: > No need for base:: Done. https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/394313002/diff/280001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:12: using base::File; On 2014/07/22 20:45:47, willchan wrote: > You don't need these using statements if everything's in the namespace, right? Done. https://codereview.chromium.org/394313002/diff/320001/base/files/file.cc File base/files/file.cc (right): https://codereview.chromium.org/394313002/diff/320001/base/files/file.cc#newc... base/files/file.cc:21: File::Region::Region() : offset(0), size(0) { This is a private ctor and can be used only by kWholeFile above. Essentially, only kWholeFile will have a size = 0 (which means no RO initializers for kWholeFile -> save 16 bytes of .rodata binary size -> just increment the .bss counter without impact on binary), hence any comparison (other == kWholeFile) would be true iff the other = kWholeFile (modulo hacking the public fields as I mentioned in my comment in file.h). https://codereview.chromium.org/394313002/diff/320001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/320001/base/files/file.h#newco... base/files/file.h:167: int64 offset; I'm a bit on the fence of this, w.r.t. keeping these fields public, or converting them to private fields + inline accessors. The latter would be a bit more bomb-proof and give more sense to the DCHEKS in the public ctor. The downside is more code clutter. On the other side we rely on clients of API to not do stupid things like Region x(0, 10); x.size = 0 //screw you WDYT?
https://codereview.chromium.org/394313002/diff/260001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:159: struct BASE_EXPORT Region { On 2014/07/23 23:17:38, Primiano Tucci wrote: > On 2014/07/22 20:45:47, willchan wrote: > > On 2014/07/21 23:55:40, willchan wrote: > > > Why is this a property of the file as opposed to the memory map? Unless > there > > > are other consumers that are interested in file regions (very possible! but > I > > > don't know of any yet), I'd prefer to keep this struct in MemoryMappedFile > > > instead. > > I'm on the fence on this. > On your side: yes, the reason why I introduce the Region concept is essentially > for mmap. > > On the other side: look at resource_bundle.cc (which is out of /base) in this CL > (lines 238-242): > It does something like this: > void ResourceBundle::AddDataPackFromFile(base::File, ScaleFactor scale_factor) { > AddDataPackFromFileRegion(file.Pass(), base::File::Region::kWholeFile, ...); > > ResourceBundle today already knows the concept of a File. ResourceBundle, > however, is oblivious of the fact that DataPack (a class it uses) will do a mmap > operation on that File (RB just passes the File along). > If i keep the Region as a File-related concept, ResourceBundle will keep to be > oblivious about the mmap implementation detail of DataPack. > If I move Region to memory_mapped_file.h, now the ResourceBundle (and DataPack) > method signatures will expose this further detail. Essentially the final > signature of DataPack would become: > > AddDataPackFromFileRegion(base::File, MemoryMappedFile::Region), which IMHO is a > bit ackward > vs the current > AddDataPackFromFileRegion(base::File, base::File::Region), which IMHO looks > better. > > What do you think? I think that you raise an interesting point that might qualify as another consumer that is interested in file regions. I'm still mulling it over. I'm trying to understand the layering with ResourceBundle. Is ResourceBundle supposed to understand the backing store for a DataPack or not? If not, why doesn't it expose ResourceBundle::AddDataPack() and allow the embedder to pass it a DataPack with whatever backing store it wants? If ResourceBundle is supposed to understand the backing store, why is it OK to understand files but not memory mapped files? I'm guessing this is a question for Tony? I guess the primary point is that it might be useful to use a file backed ResourceBundle, but not necessarily a memory mapped filed backed ResourceBundle. But would this ever actually be useful? If not, it's less mental comprehension overhead just to cut to the chase and only expose the memory mapped one. I'm open to keeping this in File. If this abstraction layer is useful, let's do it. But I'm not immediately convinced of its utility. https://codereview.chromium.org/394313002/diff/320001/base/files/file.cc File base/files/file.cc (right): https://codereview.chromium.org/394313002/diff/320001/base/files/file.cc#newc... base/files/file.cc:21: File::Region::Region() : offset(0), size(0) { On 2014/07/23 23:17:38, Primiano Tucci wrote: > This is a private ctor and can be used only by kWholeFile above. > Essentially, only kWholeFile will have a size = 0 (which means no RO > initializers for kWholeFile -> save 16 bytes of .rodata binary size -> just > increment the .bss counter without impact on binary), hence any comparison > (other == kWholeFile) would be true iff the other = kWholeFile (modulo hacking > the public fields as I mentioned in my comment in file.h). Sounds great. https://codereview.chromium.org/394313002/diff/320001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/320001/base/files/file.h#newco... base/files/file.h:167: int64 offset; On 2014/07/23 23:17:38, Primiano Tucci wrote: > I'm a bit on the fence of this, w.r.t. keeping these fields public, or > converting them to private fields + inline accessors. > The latter would be a bit more bomb-proof and give more sense to the DCHEKS in > the public ctor. > The downside is more code clutter. On the other side we rely on clients of API > to not do stupid things like Region x(0, 10); x.size = 0 //screw you > > WDYT? I think Region should be a dumb struct. Whatever function accepts Region should validate the input. https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... base/files/memory_mapped_file.cc:65: const int64 mask = static_cast<int64>(SysInfo::VMAllocationGranularity()) - 1; Can you DCHECK to make sure that this mask is never greater than 32 bits? I don't know what architecture would cause that, but it's nice to be future-proof. Otherwise, we get a truncation bug here. https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:53: DLOG(ERROR) << "Region bounds are not valid for mmap"; How about NOTREACHED()? https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:119: TEST_F(MemoryMappedFileTest, MapPartialRegionAtEnd) { Can you add another test with the partial region in the middle and not on a 1024 byte boundary?
On 2014/07/25 21:21:24, willchan wrote: > https://codereview.chromium.org/394313002/diff/260001/base/files/file.h > File base/files/file.h (right): > > https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... > base/files/file.h:159: struct BASE_EXPORT Region { > On 2014/07/23 23:17:38, Primiano Tucci wrote: > > On 2014/07/22 20:45:47, willchan wrote: > > > On 2014/07/21 23:55:40, willchan wrote: > > > > Why is this a property of the file as opposed to the memory map? Unless > > there > > > > are other consumers that are interested in file regions (very possible! > but > > I > > > > don't know of any yet), I'd prefer to keep this struct in MemoryMappedFile > > > > instead. > > > > I'm on the fence on this. > > On your side: yes, the reason why I introduce the Region concept is > essentially > > for mmap. > > > > On the other side: look at resource_bundle.cc (which is out of /base) in this > CL > > (lines 238-242): > > It does something like this: > > void ResourceBundle::AddDataPackFromFile(base::File, ScaleFactor scale_factor) > { > > AddDataPackFromFileRegion(file.Pass(), base::File::Region::kWholeFile, ...); > > > > ResourceBundle today already knows the concept of a File. ResourceBundle, > > however, is oblivious of the fact that DataPack (a class it uses) will do a > mmap > > operation on that File (RB just passes the File along). > > If i keep the Region as a File-related concept, ResourceBundle will keep to be > > oblivious about the mmap implementation detail of DataPack. > > If I move Region to memory_mapped_file.h, now the ResourceBundle (and > DataPack) > > method signatures will expose this further detail. Essentially the final > > signature of DataPack would become: > > > > AddDataPackFromFileRegion(base::File, MemoryMappedFile::Region), which IMHO is > a > > bit ackward > > vs the current > > AddDataPackFromFileRegion(base::File, base::File::Region), which IMHO looks > > better. > > > > What do you think? > > I think that you raise an interesting point that might qualify as another > consumer that is interested in file regions. I'm still mulling it over. > > I'm trying to understand the layering with ResourceBundle. Is ResourceBundle > supposed to understand the backing store for a DataPack or not? If not, why > doesn't it expose ResourceBundle::AddDataPack() and allow the embedder to pass > it a DataPack with whatever backing store it wants? If ResourceBundle is > supposed to understand the backing store, why is it OK to understand files but > not memory mapped files? > > I'm guessing this is a question for Tony? > > I guess the primary point is that it might be useful to use a file backed > ResourceBundle, but not necessarily a memory mapped filed backed ResourceBundle. > But would this ever actually be useful? If not, it's less mental comprehension > overhead just to cut to the chase and only expose the memory mapped one. > > I'm open to keeping this in File. If this abstraction layer is useful, let's do > it. But I'm not immediately convinced of its utility. I think it's OK for ResourceBundle to know about datapacks and memory mapped files. I would probably make region part of memorymappedfile until there is a reason to refactor it out into file, but I don't feel strongly one way or another.
https://codereview.chromium.org/394313002/diff/260001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:159: struct BASE_EXPORT Region { On 2014/07/25 21:21:23, willchan wrote: > On 2014/07/23 23:17:38, Primiano Tucci wrote: > > On 2014/07/22 20:45:47, willchan wrote: > > > On 2014/07/21 23:55:40, willchan wrote: > > > > Why is this a property of the file as opposed to the memory map? Unless > > there > > > > are other consumers that are interested in file regions (very possible! > but > > I > > > > don't know of any yet), I'd prefer to keep this struct in MemoryMappedFile > > > > instead. > > > > I'm on the fence on this. > > On your side: yes, the reason why I introduce the Region concept is > essentially > > for mmap. > > > > On the other side: look at resource_bundle.cc (which is out of /base) in this > CL > > (lines 238-242): > > It does something like this: > > void ResourceBundle::AddDataPackFromFile(base::File, ScaleFactor scale_factor) > { > > AddDataPackFromFileRegion(file.Pass(), base::File::Region::kWholeFile, ...); > > > > ResourceBundle today already knows the concept of a File. ResourceBundle, > > however, is oblivious of the fact that DataPack (a class it uses) will do a > mmap > > operation on that File (RB just passes the File along). > > If i keep the Region as a File-related concept, ResourceBundle will keep to be > > oblivious about the mmap implementation detail of DataPack. > > If I move Region to memory_mapped_file.h, now the ResourceBundle (and > DataPack) > > method signatures will expose this further detail. Essentially the final > > signature of DataPack would become: > > > > AddDataPackFromFileRegion(base::File, MemoryMappedFile::Region), which IMHO is > a > > bit ackward > > vs the current > > AddDataPackFromFileRegion(base::File, base::File::Region), which IMHO looks > > better. > > > > What do you think? > > I think that you raise an interesting point that might qualify as another > consumer that is interested in file regions. I'm still mulling it over. > > I'm trying to understand the layering with ResourceBundle. Is ResourceBundle > supposed to understand the backing store for a DataPack or not? If not, why > doesn't it expose ResourceBundle::AddDataPack() and allow the embedder to pass > it a DataPack with whatever backing store it wants? If ResourceBundle is > supposed to understand the backing store, why is it OK to understand files but > not memory mapped files? I was looking again at this code with fresh mind, after the weekend, and I think that: It is fine that DataPack/ResourceBundle expose the data store (i.e. the file), as that is part of the service they exhibit: they're offering the possibility of loading stuff from a file, so that's fine that their contract knows about base::File. However, if Region becomes part of MemMapFile, DataPack/ResourceBundle would start exposing unnecessary details of their internal implementation (the fact that they are mmap()-ing reather than read()-ing), which IHMO looks less nice. In other words, what if tomorrow DP/RB want to just read() the region from the file, for instance because they need to decode a protobuf from that and don't need to keep the memory around? The more I look at that, the more it feels to me that Region itself is a concept more related to a File. Anyways, I'm going to prepare a 2nd CL which is the exact copy of this, with the only exception of Region being moved to MemoryMappedFile. You guys have the final word, I'm fine either ways. I'd just kindly ask you to LGTM at this point the one you finally like the most (provided that there are no other concerns left) and let me and save another day of cross-timezone reviewing. https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... File base/files/memory_mapped_file.cc (right): https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... base/files/memory_mapped_file.cc:65: const int64 mask = static_cast<int64>(SysInfo::VMAllocationGranularity()) - 1; On 2014/07/25 21:21:23, willchan wrote: > Can you DCHECK to make sure that this mask is never greater than 32 bits? I > don't know what architecture would cause that, but it's nice to be future-proof. > Otherwise, we get a truncation bug here. Done. Even if an allocation granularity of 2/4 gigs is *kind of* coarse ;) https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:53: DLOG(ERROR) << "Region bounds are not valid for mmap"; On 2014/07/25 21:21:24, willchan wrote: > How about NOTREACHED()? Hmm technically that is reachable if the client calling this API is being stupid: If you do memory_mapped_file.Initialize(some_file, File::Region(0xffffffffffffffffLL, 10248576)), you'll hit this. I might be wrong, but I think that NOTREACHED should emphasize the concept of "if you hit this, there might be a bug / invalid assumption in *this* file (mmap..posix.cc)". What I really want to express here, instead, is: "if you hit this, you (caller) are doing something wrong". And I think a DLOG (and returning false) is more suitable, isn't it? https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... File base/files/memory_mapped_file_unittest.cc (right): https://codereview.chromium.org/394313002/diff/340001/base/files/memory_mappe... base/files/memory_mapped_file_unittest.cc:119: TEST_F(MemoryMappedFileTest, MapPartialRegionAtEnd) { On 2014/07/25 21:21:24, willchan wrote: > Can you add another test with the partial region in the middle and not on a 1024 > byte boundary? Sure. Actually, none of these boundaries should be aligned (to a 1k / page). Making all the tests unaligned (just aligned to 32b) and adding two new tests for the middle page: small (< 1 page) and large (> 1 page).
Ok, as promised I created the variant of this CL in https://codereview.chromium.org/426553005/. I organized the variant into two patchset, in order to highlight the delta w.r.t of this CL (more details in the CL descr of crrev.com/426553005). At this point, in the light of the discussions above, tell me which one you prefer. If you prefer Region being part of base::File I will just land this CL untouched. If you prefer Region being part of base::MemoryMappedFile, I will add a new PS to this CL (which will be identical to 426553005). (In any case I want this one to be the "landing" CL, so all the fruitful discussion will be kept). Thanks.
https://codereview.chromium.org/394313002/diff/260001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/394313002/diff/260001/base/files/file.h#newco... base/files/file.h:159: struct BASE_EXPORT Region { On 2014/07/28 12:52:11, Primiano Tucci wrote: > On 2014/07/25 21:21:23, willchan wrote: > > On 2014/07/23 23:17:38, Primiano Tucci wrote: > > > On 2014/07/22 20:45:47, willchan wrote: > > > > On 2014/07/21 23:55:40, willchan wrote: > > > > > Why is this a property of the file as opposed to the memory map? Unless > > > there > > > > > are other consumers that are interested in file regions (very possible! > > but > > > I > > > > > don't know of any yet), I'd prefer to keep this struct in > MemoryMappedFile > > > > > instead. > > > > > > I'm on the fence on this. > > > On your side: yes, the reason why I introduce the Region concept is > > essentially > > > for mmap. > > > > > > On the other side: look at resource_bundle.cc (which is out of /base) in > this > > CL > > > (lines 238-242): > > > It does something like this: > > > void ResourceBundle::AddDataPackFromFile(base::File, ScaleFactor > scale_factor) > > { > > > AddDataPackFromFileRegion(file.Pass(), base::File::Region::kWholeFile, > ...); > > > > > > ResourceBundle today already knows the concept of a File. ResourceBundle, > > > however, is oblivious of the fact that DataPack (a class it uses) will do a > > mmap > > > operation on that File (RB just passes the File along). > > > If i keep the Region as a File-related concept, ResourceBundle will keep to > be > > > oblivious about the mmap implementation detail of DataPack. > > > If I move Region to memory_mapped_file.h, now the ResourceBundle (and > > DataPack) > > > method signatures will expose this further detail. Essentially the final > > > signature of DataPack would become: > > > > > > AddDataPackFromFileRegion(base::File, MemoryMappedFile::Region), which IMHO > is > > a > > > bit ackward > > > vs the current > > > AddDataPackFromFileRegion(base::File, base::File::Region), which IMHO looks > > > better. > > > > > > What do you think? > > > > I think that you raise an interesting point that might qualify as another > > consumer that is interested in file regions. I'm still mulling it over. > > > > I'm trying to understand the layering with ResourceBundle. Is ResourceBundle > > supposed to understand the backing store for a DataPack or not? If not, why > > doesn't it expose ResourceBundle::AddDataPack() and allow the embedder to pass > > it a DataPack with whatever backing store it wants? If ResourceBundle is > > supposed to understand the backing store, why is it OK to understand files but > > not memory mapped files? > > I was looking again at this code with fresh mind, after the weekend, and I think > that: > It is fine that DataPack/ResourceBundle expose the data store (i.e. the file), > as that is part of the service they exhibit: they're offering the possibility of > loading stuff from a file, so that's fine that their contract knows about > base::File. > However, if Region becomes part of MemMapFile, DataPack/ResourceBundle would > start exposing unnecessary details of their internal implementation (the fact > that they are mmap()-ing reather than read()-ing), which IHMO looks less nice. > > In other words, what if tomorrow DP/RB want to just read() the region from the > file, for instance because they need to decode a protobuf from that and don't > need to keep the memory around? > The more I look at that, the more it feels to me that Region itself is a concept > more related to a File. Hm, well this require refactoring all uses today or introducing a new, separate API method, right? The existing ResourceBundle API makes the assumption that the file region encoding already matches what it'd be in memory. You can't support a new encoding in the current API. That said, I did acknowledge something similar to this above: """ I guess the primary point is that it might be useful to use a file backed ResourceBundle, but not necessarily a memory mapped filed backed ResourceBundle. But would this ever actually be useful? If not, it's less mental comprehension overhead just to cut to the chase and only expose the memory mapped one. """ Generally, for base/ stuff, we err on the side of not introducing new abstractions in the *possibility* of future use. We wait until the new use actually happens. We'd let it in only use we knew it was going to be used in a followup CL. Furthermore, I'd assert that if ResourceBundle were going to introduce this kind of flexibility in the future, it should just take in the DataPack rather than have all the input parameters for the DataPack's construction in the ResourceBundle's API. ResourceBundle doesn't need to know how the DataPack's are constructed. DataPacks could be constructed with the appropriate backend (memory map, protobuffer, etc) and passed in post-construction to the ResourceBundle. > > Anyways, I'm going to prepare a 2nd CL which is the exact copy of this, with the > only exception of Region being moved to MemoryMappedFile. > You guys have the final word, I'm fine either ways. > I'd just kindly ask you to LGTM at this point the one you finally like the most > (provided that there are no other concerns left) and let me and save another day > of cross-timezone reviewing.
Got it. Thanks for the quick response! By LGTemming the other CL I assume you are fine moving Region MMfile, right? Patch set 12 here is === crrev.com/426553005, where Region has been moved to MemoryMappedFile. Closing the variant and landing this one, unless there are other concerns.
LGTM On Mon, Jul 28, 2014 at 8:08 AM, <primiano@chromium.org> wrote: > Got it. Thanks for the quick response! > By LGTemming the other CL I assume you are fine moving Region MMfile, > right? > > Patch set 12 here is === crrev.com/426553005, where Region has been moved > to > MemoryMappedFile. > Closing the variant and landing this one, unless there are other concerns. > > > https://codereview.chromium.org/394313002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/394313002/370014
Message was sent while issue was closed.
Change committed as 285945
Message was sent while issue was closed.
https://codereview.chromium.org/394313002/diff/370014/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/394313002/diff/370014/base/files/memory_mappe... base/files/memory_mapped_file.h:29: static const Region kWholeFile; Doesn't this result in a static constructor, which is disallowed?
Message was sent while issue was closed.
https://codereview.chromium.org/394313002/diff/370014/base/files/memory_mappe... File base/files/memory_mapped_file.h (right): https://codereview.chromium.org/394313002/diff/370014/base/files/memory_mappe... base/files/memory_mapped_file.h:29: static const Region kWholeFile; On 2014/07/28 22:07:43, sky wrote: > Doesn't this result in a static constructor, which is disallowed? We had an offline discussion with Will on this point (I had the same doubt), and my understanding is: if the only thing the ctor does is initializing POD fields (in this specific case, to 0), it would not be a problem. Essentially here kWholeFile is a struct with two 0-initialized fields. I'd expect kWholeFile to be just a carve-out of .bss. I did a quick test by turning kWholeFile into a static method which returns a new Region every time, and if I diff the program headers, I can see a 16 bytes delta in bss (for libbase.so): With kWholeFile being a static const: [27] .bss NOBITS 00000000003e3660 3e2660 002714 00 WA 0 0 16 With kWholeFile being a method which retuns a new object [27] .bss NOBITS 00000000003e3660 3e2660 002704 00 WA 0 0 16 (Size column is the 002714, 002704) Also, I assume we have checks in place for this (I am sure I've seen clang barfing for at-exit destructors). Does it sound right?
Yeah, I was assuming our static initializer tests would barf if Primiano got it wrong :) I agree, however we do it, adding a static initializer is unacceptable. I think it's fine, but if it does add a static initializer somewhere, let's fix that. On Tue, Jul 29, 2014 at 2:05 AM, <primiano@chromium.org> wrote: > > https://codereview.chromium.org/394313002/diff/370014/ > base/files/memory_mapped_file.h > File base/files/memory_mapped_file.h (right): > > https://codereview.chromium.org/394313002/diff/370014/ > base/files/memory_mapped_file.h#newcode29 > base/files/memory_mapped_file.h:29: static const Region kWholeFile; > On 2014/07/28 22:07:43, sky wrote: > >> Doesn't this result in a static constructor, which is disallowed? >> > We had an offline discussion with Will on this point (I had the same > doubt), and my understanding is: if the only thing the ctor does is > initializing POD fields (in this specific case, to 0), it would not be a > problem. Essentially here kWholeFile is a struct with two 0-initialized > fields. I'd expect kWholeFile to be just a carve-out of .bss. > > I did a quick test by turning kWholeFile into a static method which > returns a new Region every time, and if I diff the program headers, I > can see a 16 bytes delta in bss (for libbase.so): > > With kWholeFile being a static const: > [27] .bss NOBITS 00000000003e3660 3e2660 002714 00 > WA 0 0 16 > > With kWholeFile being a method which retuns a new object > [27] .bss NOBITS 00000000003e3660 3e2660 002704 00 > WA 0 0 16 > > (Size column is the 002714, 002704) > Also, I assume we have checks in place for this (I am sure I've seen > clang barfing for at-exit destructors). > > Does it sound right? > > https://codereview.chromium.org/394313002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/07/29 15:17:35, willchan wrote: > Yeah, I was assuming our static initializer tests would barf if Primiano > got it wrong :) I agree, however we do it, adding a static initializer is > unacceptable. I think it's fine, but if it does add a static initializer > somewhere, let's fix that. > > > On Tue, Jul 29, 2014 at 2:05 AM, <mailto:primiano@chromium.org> wrote: > > > > > https://codereview.chromium.org/394313002/diff/370014/ > > base/files/memory_mapped_file.h > > File base/files/memory_mapped_file.h (right): > > > > https://codereview.chromium.org/394313002/diff/370014/ > > base/files/memory_mapped_file.h#newcode29 > > base/files/memory_mapped_file.h:29: static const Region kWholeFile; > > On 2014/07/28 22:07:43, sky wrote: > > > >> Doesn't this result in a static constructor, which is disallowed? > >> > > We had an offline discussion with Will on this point (I had the same > > doubt), and my understanding is: if the only thing the ctor does is > > initializing POD fields (in this specific case, to 0), it would not be a > > problem. Essentially here kWholeFile is a struct with two 0-initialized > > fields. I'd expect kWholeFile to be just a carve-out of .bss. > > > > I did a quick test by turning kWholeFile into a static method which > > returns a new Region every time, and if I diff the program headers, I > > can see a 16 bytes delta in bss (for libbase.so): > > > > With kWholeFile being a static const: > > [27] .bss NOBITS 00000000003e3660 3e2660 002714 00 > > WA 0 0 16 > > > > With kWholeFile being a method which retuns a new object > > [27] .bss NOBITS 00000000003e3660 3e2660 002704 00 > > WA 0 0 16 > > > > (Size column is the 002714, 002704) > > Also, I assume we have checks in place for this (I am sure I've seen > > clang barfing for at-exit destructors). > > > > Does it sound right? > > > > https://codereview.chromium.org/394313002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I thought the right thing here was to have no constructor. That way no static initialize. I'm a bit hazy on this though. Nico knows this more than I though, so +nico.
It should be fine in practice, but we use base::LINKER_INITIALIZED in other places where we do this for documentation purposes, so you might want to use that here too. On Tue, Jul 29, 2014 at 10:52 AM, <sky@chromium.org> wrote: > On 2014/07/29 15:17:35, willchan wrote: > >> Yeah, I was assuming our static initializer tests would barf if Primiano >> got it wrong :) I agree, however we do it, adding a static initializer is >> unacceptable. I think it's fine, but if it does add a static initializer >> somewhere, let's fix that. >> > > > On Tue, Jul 29, 2014 at 2:05 AM, <mailto:primiano@chromium.org> wrote: >> > > > >> > https://codereview.chromium.org/394313002/diff/370014/ >> > base/files/memory_mapped_file.h >> > File base/files/memory_mapped_file.h (right): >> > >> > https://codereview.chromium.org/394313002/diff/370014/ >> > base/files/memory_mapped_file.h#newcode29 >> > base/files/memory_mapped_file.h:29: static const Region kWholeFile; >> > On 2014/07/28 22:07:43, sky wrote: >> > >> >> Doesn't this result in a static constructor, which is disallowed? >> >> >> > We had an offline discussion with Will on this point (I had the same >> > doubt), and my understanding is: if the only thing the ctor does is >> > initializing POD fields (in this specific case, to 0), it would not be a >> > problem. Essentially here kWholeFile is a struct with two 0-initialized >> > fields. I'd expect kWholeFile to be just a carve-out of .bss. >> > >> > I did a quick test by turning kWholeFile into a static method which >> > returns a new Region every time, and if I diff the program headers, I >> > can see a 16 bytes delta in bss (for libbase.so): >> > >> > With kWholeFile being a static const: >> > [27] .bss NOBITS 00000000003e3660 3e2660 002714 00 >> > WA 0 0 16 >> > >> > With kWholeFile being a method which retuns a new object >> > [27] .bss NOBITS 00000000003e3660 3e2660 002704 00 >> > WA 0 0 16 >> > >> > (Size column is the 002714, 002704) >> > Also, I assume we have checks in place for this (I am sure I've seen >> > clang barfing for at-exit destructors). >> > >> > Does it sound right? >> > >> > https://codereview.chromium.org/394313002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > I thought the right thing here was to have no constructor. That way no > static > initialize. I'm a bit hazy on this though. Nico knows this more than I > though, > so +nico. > > https://codereview.chromium.org/394313002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/07/29 18:12:07, Nico (away) wrote: > It should be fine in practice, but we use base::LINKER_INITIALIZED in other > places where we do this for documentation purposes, so you might want to > use that here too. > > > On Tue, Jul 29, 2014 at 10:52 AM, <mailto:sky@chromium.org> wrote: > > > On 2014/07/29 15:17:35, willchan wrote: > > > >> Yeah, I was assuming our static initializer tests would barf if Primiano > >> got it wrong :) I agree, however we do it, adding a static initializer is > >> unacceptable. I think it's fine, but if it does add a static initializer > >> somewhere, let's fix that. > >> > > > > > > On Tue, Jul 29, 2014 at 2:05 AM, <mailto:primiano@chromium.org> wrote: > >> > > > > > > >> > https://codereview.chromium.org/394313002/diff/370014/ > >> > base/files/memory_mapped_file.h > >> > File base/files/memory_mapped_file.h (right): > >> > > >> > https://codereview.chromium.org/394313002/diff/370014/ > >> > base/files/memory_mapped_file.h#newcode29 > >> > base/files/memory_mapped_file.h:29: static const Region kWholeFile; > >> > On 2014/07/28 22:07:43, sky wrote: > >> > > >> >> Doesn't this result in a static constructor, which is disallowed? > >> >> > >> > We had an offline discussion with Will on this point (I had the same > >> > doubt), and my understanding is: if the only thing the ctor does is > >> > initializing POD fields (in this specific case, to 0), it would not be a > >> > problem. Essentially here kWholeFile is a struct with two 0-initialized > >> > fields. I'd expect kWholeFile to be just a carve-out of .bss. > >> > > >> > I did a quick test by turning kWholeFile into a static method which > >> > returns a new Region every time, and if I diff the program headers, I > >> > can see a 16 bytes delta in bss (for libbase.so): > >> > > >> > With kWholeFile being a static const: > >> > [27] .bss NOBITS 00000000003e3660 3e2660 002714 00 > >> > WA 0 0 16 > >> > > >> > With kWholeFile being a method which retuns a new object > >> > [27] .bss NOBITS 00000000003e3660 3e2660 002704 00 > >> > WA 0 0 16 > >> > > >> > (Size column is the 002714, 002704) > >> > Also, I assume we have checks in place for this (I am sure I've seen > >> > clang barfing for at-exit destructors). > >> > > >> > Does it sound right? > >> > > >> > https://codereview.chromium.org/394313002/ > >> > > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > I thought the right thing here was to have no constructor. That way no > > static > > initialize. I'm a bit hazy on this though. Nico knows this more than I > > though, > > so +nico. > > > > https://codereview.chromium.org/394313002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. FTR I checked with tools/linux/dump-static-initializers.py and I confirm that the Release binary (I tried just doing a component build of libbase.so) doesn't have any static initializer. The debug version, instead, shows it. I think that's because the compiler doesn't try to inline the empty body of the ctor and doesn't realize that there isn't anything to call. Anyways, I'm adding the LINKER_INITIALIZED as Nico suggested here: https://codereview.chromium.org/428883012/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
