Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6597038:
Implementation of an STL compatible allocator for Courgette on Windows.... (Closed)
|
Created:
9 years, 9 months ago by tommi (sloooow) - chröme Modified:
9 years, 7 months ago CC:
chromium-reviews, amit Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplementation of an STL compatible allocator for Courgette on Windows.
This is to better handle low memory situations when applying a differential
patch to a large Chrome setup.
For reading input files, I'm also switching to using memory mapped files
instead of ReadFileToString to reduce the load on the heap.
TEST=courgette.exe should succeed in applying a patch between two
chrome 10.x archives on an XP machine with 180MB of physical memory and
no page file.
BUG=72459, 73209
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76320
Patch Set 1 #Patch Set 2 : '' #
Total comments: 24
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 3
Patch Set 5 : '' #
Total comments: 19
Patch Set 6 : '' #
Messages
Total messages: 12 (0 generated)
Just passing through... http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.cc File courgette/memory_allocator.cc (right): http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:34: file_util::CreateTemporaryFile(&path); what if this returns false? http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:40: file_ = base::CreatePlatformFile(path, flags, &created, &error_code); PLOG_IF(ERROR, file_ == base::kInvalidPlatformFileValue) << ...? http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:117: size += sizeof(TempMapping*); // NOLINT Style guide says "Use sizeof(varname) instead of sizeof(type) whenever possible.", so how about sizeof(this)? http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:131: mem += sizeof(TempMapping*); // NOLINT sizeof(this)? http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h File courgette/memory_allocator.h (right): http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:97: class MemoryAllocator : public std::_Allocator_base<T> { Does deriving from an implementation detail of Microsoft's STL really simplify things? Seems like this adds a layer of fragility since it relies on undocumented behavior. See http://blogs.msdn.com/b/vcblog/archive/2008/08/28/the-mallocator.aspx for an example of a fairly minimal allocator. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation size that well attempt. well -> we'll http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:143: TempMapping* mapping = TempMapping::GetMappingFromPtr(mem); DCHECK_EQ(FILE_ALLOCATION, mem[0])? http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:149: size_type bytes = count * sizeof(T); check for overflow (count > max_size()), throwing std::length_error if so. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:160: mapping->Initialize(bytes + 1); what if this returns false? http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:164: return reinterpret_cast<pointer>(mem + 1); C++-03 20.4.1.1 says that this must return "a pointer to the initial element of an array of storage of size n * sizeof(T), aligned appropriately for objects of type T." this impl will only satisfy the alignment requirement when sizeof(T) == sizeof(uint8). please document and COMPILE_ASSERT that if it's okay, or fix the impl if it's not. Also consider that returning memory that isn't aligned to (at least) a machine word could have perf implications if you're doing memcpy/memmove/etc. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:172: std::_Construct(_Ptr, _Val); Another implementation detail used here. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:176: std::_Destroy(_Ptr); And here.
On 2011/02/28 15:06:41, grt wrote: > Just passing through... I forgot one comment about this part: > ...180MB of physical memory and no page file. Tres cool, compadre!
thanks for driving by http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.cc File courgette/memory_allocator.cc (right): http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:34: file_util::CreateTemporaryFile(&path); On 2011/02/28 15:06:41, grt wrote: > what if this returns false? Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:40: file_ = base::CreatePlatformFile(path, flags, &created, &error_code); On 2011/02/28 15:06:41, grt wrote: > PLOG_IF(ERROR, file_ == base::kInvalidPlatformFileValue) << ...? Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:117: size += sizeof(TempMapping*); // NOLINT On 2011/02/28 15:06:41, grt wrote: > Style guide says "Use sizeof(varname) instead of sizeof(type) whenever > possible.", so how about sizeof(this)? Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.c... courgette/memory_allocator.cc:131: mem += sizeof(TempMapping*); // NOLINT On 2011/02/28 15:06:41, grt wrote: > sizeof(this)? Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h File courgette/memory_allocator.h (right): http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:97: class MemoryAllocator : public std::_Allocator_base<T> { removed. Also updated the types accordingly. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation size that well attempt. On 2011/02/28 15:06:41, grt wrote: > well -> we'll Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:143: TempMapping* mapping = TempMapping::GetMappingFromPtr(mem); On 2011/02/28 15:06:41, grt wrote: > DCHECK_EQ(FILE_ALLOCATION, mem[0])? Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:149: size_type bytes = count * sizeof(T); On 2011/02/28 15:06:41, grt wrote: > check for overflow (count > max_size()), throwing std::length_error if so. Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:160: mapping->Initialize(bytes + 1); On 2011/02/28 15:06:41, grt wrote: > what if this returns false? added check+throw bad_alloc http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:164: return reinterpret_cast<pointer>(mem + 1); fixed so that heap allocations are always correctly aligned and file allocations are allocated on a machine word boundary. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:172: std::_Construct(_Ptr, _Val); On 2011/02/28 15:06:41, grt wrote: > Another implementation detail used here. Done. http://codereview.chromium.org/6597038/diff/5002/courgette/memory_allocator.h... courgette/memory_allocator.h:176: std::_Destroy(_Ptr); On 2011/02/28 15:06:41, grt wrote: > And here. Done.
LGTM http://codereview.chromium.org/6597038/diff/7003/courgette/memory_allocator.h File courgette/memory_allocator.h (right): http://codereview.chromium.org/6597038/diff/7003/courgette/memory_allocator.h... courgette/memory_allocator.h:154: count++; check for overflow. http://codereview.chromium.org/6597038/diff/7003/courgette/memory_allocator.h... courgette/memory_allocator.h:171: if (!mapping->Initialize(bytes + 1)) { Initialize(bytes) now that bytes includes the extra element?
http://codereview.chromium.org/6597038/diff/7003/courgette/memory_allocator.h File courgette/memory_allocator.h (right): http://codereview.chromium.org/6597038/diff/7003/courgette/memory_allocator.h... courgette/memory_allocator.h:171: if (!mapping->Initialize(bytes + 1)) { On 2011/02/28 18:08:34, grt wrote: > Initialize(bytes) now that bytes includes the extra element? Doh, thanks
I'm excited about this work. I have lots of comments but I'm basically OK with it. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.cc File courgette/memory_allocator.cc (right): http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... courgette/memory_allocator.cc:88: write[size - 1] = 0; What does this do? Does not TempFile::SetSize somehow not do the job? http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... courgette/memory_allocator.cc:120: size += sizeof(this); FYI. The assumption here is that the alignment of pointers (this) is as strict or stricter than the alignment of the element type. This is not always true, e.g. __m128 has 16-byte alignment. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... courgette/memory_allocator.cc:143: ret = reinterpret_cast<TempMapping**>(mem)[-1]; It would be nice if the place that sets this was obviously the same calculation. There are three functions that have the magic offset, expressed in different ways (+ sizeof vs [-1]). http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h File courgette/memory_allocator.h (right): http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:19: // is deleted when the file handle is closed. Add somewhere: Since the file will be used as backing for a memory allocation, it will never be so big that size_t cannot represent it's size. Otherwise someone will wonder why the size is not [u]int64. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:71: class TempMapping { The two classes before this are all implementation detail. They could be moved to the .cc http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:96: // or if a heap allocation fails. I would add a comment about the 'why', something like: Allocating the memory as a mapping of a temporary file solves the problem that there might not be enough physical memory and pagefile to support the allocation. This can happen because these resources are too small, or already committed to other processes. Provided there is enough disk, the temporary file acts like a pagefile that other processes can't access. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:109: // deallocate it. I think this is OK, but I would have done this slightly differently: 1. Keep a 'global' data structure mapping address to TempMapping. 2. Normal allocations are not in the table. Keeping the allocation metadata separate has several nice properties: 1. The allocation constraints are automatically satisfied by the underlying allocators (operator new/filesystem). 2. If I ask for a nice round number like 5MB I don't get an extra cluster allocated. 3. The allocation metadata is very diffuse. A side table has greater cache and/or page density. You don't page in the first page of the file just to close the file. (Ditto for cache lines but that is not the issue here.) 4. Separated metadata is less likely to get trashed on buffer underrun. Normally with a separated-metadata allocator there would be some effort to make the lookup on deallocation fast. Here a simple map<void*, TempMapping*> would be fine - we do at most a couple of thousand allocations. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation size that we'll attempt. Say why it is 4MB, rather than, say, 128k or 32MB.
http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.cc File courgette/memory_allocator.cc (right): http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... courgette/memory_allocator.cc:88: write[size - 1] = 0; On 2011/02/28 20:28:38, sra1 wrote: > What does this do? > Does not TempFile::SetSize somehow not do the job? oops, removed, I had this in while debugging. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... courgette/memory_allocator.cc:120: size += sizeof(this); On 2011/02/28 20:28:38, sra1 wrote: > FYI. The assumption here is that the alignment of pointers (this) is as strict > or stricter than the alignment of the element type. This is not always true, > e.g. __m128 has 16-byte alignment. Yes, for large objects the alignment will be broken. However, this is not currently a problem. Are you OK with me putting down a todo for myself to fix this? http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... courgette/memory_allocator.cc:143: ret = reinterpret_cast<TempMapping**>(mem)[-1]; On 2011/02/28 20:28:38, sra1 wrote: > It would be nice if the place that sets this was obviously the same calculation. > There are three functions that have the magic offset, expressed in different > ways (+ sizeof vs [-1]). I switched Initialize() to also use []. I don't see an easy way to consolidate these all in a single function and still maintain readability, but if you have suggestions I can incorporate them. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h File courgette/memory_allocator.h (right): http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:19: // is deleted when the file handle is closed. On 2011/02/28 20:28:38, sra1 wrote: > Add somewhere: Since the file will be used as backing for a memory allocation, > it will never be so big that size_t cannot represent it's size. > Otherwise someone will wonder why the size is not [u]int64. Done. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:71: class TempMapping { On 2011/02/28 20:28:38, sra1 wrote: > The two classes before this are all implementation detail. They could be moved > to the .cc This class holds member variables (not pointers) of those classes they need to be there. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:96: // or if a heap allocation fails. On 2011/02/28 20:28:38, sra1 wrote: > I would add a comment about the 'why', something like: > > Allocating the memory as a mapping of a temporary file solves the problem that > there might not be enough physical memory and pagefile to support the > allocation. This can happen because these resources are too small, or already > committed to other processes. Provided there is enough disk, the temporary file > acts like a pagefile that other processes can't access. Done. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:109: // deallocate it. I thought about that kind of approach but went with this approach because it was simpler and didn't depend on other allocations. However, you have a point and I'm happy to oblige. Do you mind that I do that on a separate changelist and likely not in time for the push this week? http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation size that we'll attempt. On 2011/02/28 20:28:38, sra1 wrote: > Say why it is 4MB, rather than, say, 128k or 32MB. To be honest this is a finger-in-the air estimate based on non scientific observation. Let me gather some numbers and pick something that I can more easily justify. As for 32MB, that's likely too big. I ran tests with 40MB as the cliff and it caused other heap allocations to start failing in low memory situatons.
So, it looks like around 5MB is a reasonable threshold. At 5170804 bytes, there are 10 allocations and lower than that there are 6362 allocations. There are 17 allocations for more than 5MB (count is nr of allocations of this size, total is the total nr of allocations): size=7972438 count=2 total=6374 size=11958657 count=2 total=6376 size=17937985 count=2 total=6378 size=19664320 count=1 total=6379 size=19667676 count=1 total=6380 size=23085856 count=1 total=6381 size=26906977 count=2 total=6383 size=34628783 count=1 total=6384 size=51943174 count=1 total=6385 size=54377232 count=1 total=6386 size=54521824 count=1 total=6387 size=89684176 count=1 total=6388 size=142957040 count=1 total=6389 So, if I change the cliff to be 5MB, we'll back 17 allocations with a file and all other allocations come from the heap unless heap allocation fails. Sounds good? On Mon, Feb 28, 2011 at 4:17 PM, <tommi@chromium.org> wrote: > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.cc > File courgette/memory_allocator.cc (right): > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... > courgette/memory_allocator.cc:88: write[size - 1] = 0; > On 2011/02/28 20:28:38, sra1 wrote: > >> What does this do? >> Does not TempFile::SetSize somehow not do the job? >> > > oops, removed, I had this in while debugging. > > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... > courgette/memory_allocator.cc:120: size += sizeof(this); > On 2011/02/28 20:28:38, sra1 wrote: > >> FYI. The assumption here is that the alignment of pointers (this) is >> > as strict > >> or stricter than the alignment of the element type. This is not >> > always true, > >> e.g. __m128 has 16-byte alignment. >> > > Yes, for large objects the alignment will be broken. However, this is > not currently a problem. Are you OK with me putting down a todo for > myself to fix this? > > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... > courgette/memory_allocator.cc:143: ret = > reinterpret_cast<TempMapping**>(mem)[-1]; > On 2011/02/28 20:28:38, sra1 wrote: > >> It would be nice if the place that sets this was obviously the same >> > calculation. > >> There are three functions that have the magic offset, expressed in >> > different > >> ways (+ sizeof vs [-1]). >> > > I switched Initialize() to also use []. > > I don't see an easy way to consolidate these all in a single function > and still maintain readability, but if you have suggestions I can > incorporate them. > > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h > File courgette/memory_allocator.h (right): > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... > courgette/memory_allocator.h:19: // is deleted when the file handle is > closed. > On 2011/02/28 20:28:38, sra1 wrote: > >> Add somewhere: Since the file will be used as backing for a memory >> > allocation, > >> it will never be so big that size_t cannot represent it's size. >> Otherwise someone will wonder why the size is not [u]int64. >> > > Done. > > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... > courgette/memory_allocator.h:71: class TempMapping { > On 2011/02/28 20:28:38, sra1 wrote: > >> The two classes before this are all implementation detail. They could >> > be moved > >> to the .cc >> > > This class holds member variables (not pointers) of those classes they > need to be there. > > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... > courgette/memory_allocator.h:96: // or if a heap allocation fails. > On 2011/02/28 20:28:38, sra1 wrote: > >> I would add a comment about the 'why', something like: >> > > Allocating the memory as a mapping of a temporary file solves the >> > problem that > >> there might not be enough physical memory and pagefile to support the >> allocation. This can happen because these resources are too small, or >> > already > >> committed to other processes. Provided there is enough disk, the >> > temporary file > >> acts like a pagefile that other processes can't access. >> > > Done. > > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... > courgette/memory_allocator.h:109: // deallocate it. > I thought about that kind of approach but went with this approach > because it was simpler and didn't depend on other allocations. However, > you have a point and I'm happy to oblige. > Do you mind that I do that on a separate changelist and likely not in > time for the push this week? > > > > http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... > courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation > size that we'll attempt. > On 2011/02/28 20:28:38, sra1 wrote: > >> Say why it is 4MB, rather than, say, 128k or 32MB. >> > > To be honest this is a finger-in-the air estimate based on non > scientific observation. Let me gather some numbers and pick something > that I can more easily justify. As for 32MB, that's likely too big. I > ran tests with 40MB as the cliff and it caused other heap allocations to > start failing in low memory situatons. > > > http://codereview.chromium.org/6597038/ >
here's a more coarse breakdown of all the allocations: <1MB=6265 <2MB=53 <3MB=21 <4MB=20 <5MB=3 <6MB=10 (these would be allocated on the heap, but below as files) <8MB=2 <12MB=2 <18MB=2 <19MB=2 <23MB=1 <26MB=2 <34MB=1 <50MB=1 <52MB=2 <86MB=1 <136MB=1 On Mon, Feb 28, 2011 at 4:52 PM, Tommi <tommi@chromium.org> wrote: > So, it looks like around 5MB is a reasonable threshold. > At 5170804 bytes, there are 10 allocations and lower than that there are > 6362 allocations. > There are 17 allocations for more than 5MB (count is nr of allocations of > this size, total is the total nr of allocations): > > size=7972438 count=2 total=6374 > size=11958657 count=2 total=6376 > size=17937985 count=2 total=6378 > size=19664320 count=1 total=6379 > size=19667676 count=1 total=6380 > size=23085856 count=1 total=6381 > size=26906977 count=2 total=6383 > size=34628783 count=1 total=6384 > size=51943174 count=1 total=6385 > size=54377232 count=1 total=6386 > size=54521824 count=1 total=6387 > size=89684176 count=1 total=6388 > size=142957040 count=1 total=6389 > > So, if I change the cliff to be 5MB, we'll back 17 allocations with a file > and all other allocations come from the heap unless heap allocation fails. > Sounds good? > > On Mon, Feb 28, 2011 at 4:17 PM, <tommi@chromium.org> wrote: > >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.cc >> File courgette/memory_allocator.cc (right): >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... >> courgette/memory_allocator.cc:88: write[size - 1] = 0; >> On 2011/02/28 20:28:38, sra1 wrote: >> >>> What does this do? >>> Does not TempFile::SetSize somehow not do the job? >>> >> >> oops, removed, I had this in while debugging. >> >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... >> courgette/memory_allocator.cc:120: size += sizeof(this); >> On 2011/02/28 20:28:38, sra1 wrote: >> >>> FYI. The assumption here is that the alignment of pointers (this) is >>> >> as strict >> >>> or stricter than the alignment of the element type. This is not >>> >> always true, >> >>> e.g. __m128 has 16-byte alignment. >>> >> >> Yes, for large objects the alignment will be broken. However, this is >> not currently a problem. Are you OK with me putting down a todo for >> myself to fix this? >> >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... >> courgette/memory_allocator.cc:143: ret = >> reinterpret_cast<TempMapping**>(mem)[-1]; >> On 2011/02/28 20:28:38, sra1 wrote: >> >>> It would be nice if the place that sets this was obviously the same >>> >> calculation. >> >>> There are three functions that have the magic offset, expressed in >>> >> different >> >>> ways (+ sizeof vs [-1]). >>> >> >> I switched Initialize() to also use []. >> >> I don't see an easy way to consolidate these all in a single function >> and still maintain readability, but if you have suggestions I can >> incorporate them. >> >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h >> File courgette/memory_allocator.h (right): >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >> courgette/memory_allocator.h:19: // is deleted when the file handle is >> closed. >> On 2011/02/28 20:28:38, sra1 wrote: >> >>> Add somewhere: Since the file will be used as backing for a memory >>> >> allocation, >> >>> it will never be so big that size_t cannot represent it's size. >>> Otherwise someone will wonder why the size is not [u]int64. >>> >> >> Done. >> >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >> courgette/memory_allocator.h:71: class TempMapping { >> On 2011/02/28 20:28:38, sra1 wrote: >> >>> The two classes before this are all implementation detail. They could >>> >> be moved >> >>> to the .cc >>> >> >> This class holds member variables (not pointers) of those classes they >> need to be there. >> >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >> courgette/memory_allocator.h:96: // or if a heap allocation fails. >> On 2011/02/28 20:28:38, sra1 wrote: >> >>> I would add a comment about the 'why', something like: >>> >> >> Allocating the memory as a mapping of a temporary file solves the >>> >> problem that >> >>> there might not be enough physical memory and pagefile to support the >>> allocation. This can happen because these resources are too small, or >>> >> already >> >>> committed to other processes. Provided there is enough disk, the >>> >> temporary file >> >>> acts like a pagefile that other processes can't access. >>> >> >> Done. >> >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >> courgette/memory_allocator.h:109: // deallocate it. >> I thought about that kind of approach but went with this approach >> because it was simpler and didn't depend on other allocations. However, >> you have a point and I'm happy to oblige. >> Do you mind that I do that on a separate changelist and likely not in >> time for the push this week? >> >> >> >> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >> courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation >> size that we'll attempt. >> On 2011/02/28 20:28:38, sra1 wrote: >> >>> Say why it is 4MB, rather than, say, 128k or 32MB. >>> >> >> To be honest this is a finger-in-the air estimate based on non >> scientific observation. Let me gather some numbers and pick something >> that I can more easily justify. As for 32MB, that's likely too big. I >> ran tests with 40MB as the cliff and it caused other heap allocations to >> start failing in low memory situatons. >> >> >> http://codereview.chromium.org/6597038/ >> > >
latest changes uploaded. please take another look. On Mon, Feb 28, 2011 at 5:23 PM, Tommi <tommi@chromium.org> wrote: > here's a more coarse breakdown of all the allocations: > > <1MB=6265 > <2MB=53 > <3MB=21 > <4MB=20 > <5MB=3 > <6MB=10 (these would be allocated on the heap, but below as files) > <8MB=2 > <12MB=2 > <18MB=2 > <19MB=2 > <23MB=1 > <26MB=2 > <34MB=1 > <50MB=1 > <52MB=2 > <86MB=1 > <136MB=1 > > On Mon, Feb 28, 2011 at 4:52 PM, Tommi <tommi@chromium.org> wrote: > >> So, it looks like around 5MB is a reasonable threshold. >> At 5170804 bytes, there are 10 allocations and lower than that there are >> 6362 allocations. >> There are 17 allocations for more than 5MB (count is nr of allocations of >> this size, total is the total nr of allocations): >> >> size=7972438 count=2 total=6374 >> size=11958657 count=2 total=6376 >> size=17937985 count=2 total=6378 >> size=19664320 count=1 total=6379 >> size=19667676 count=1 total=6380 >> size=23085856 count=1 total=6381 >> size=26906977 count=2 total=6383 >> size=34628783 count=1 total=6384 >> size=51943174 count=1 total=6385 >> size=54377232 count=1 total=6386 >> size=54521824 count=1 total=6387 >> size=89684176 count=1 total=6388 >> size=142957040 count=1 total=6389 >> >> So, if I change the cliff to be 5MB, we'll back 17 allocations with a file >> and all other allocations come from the heap unless heap allocation fails. >> Sounds good? >> >> On Mon, Feb 28, 2011 at 4:17 PM, <tommi@chromium.org> wrote: >> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.cc >>> File courgette/memory_allocator.cc (right): >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... >>> courgette/memory_allocator.cc:88: write[size - 1] = 0; >>> On 2011/02/28 20:28:38, sra1 wrote: >>> >>>> What does this do? >>>> Does not TempFile::SetSize somehow not do the job? >>>> >>> >>> oops, removed, I had this in while debugging. >>> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... >>> courgette/memory_allocator.cc:120: size += sizeof(this); >>> On 2011/02/28 20:28:38, sra1 wrote: >>> >>>> FYI. The assumption here is that the alignment of pointers (this) is >>>> >>> as strict >>> >>>> or stricter than the alignment of the element type. This is not >>>> >>> always true, >>> >>>> e.g. __m128 has 16-byte alignment. >>>> >>> >>> Yes, for large objects the alignment will be broken. However, this is >>> not currently a problem. Are you OK with me putting down a todo for >>> myself to fix this? >>> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... >>> courgette/memory_allocator.cc:143: ret = >>> reinterpret_cast<TempMapping**>(mem)[-1]; >>> On 2011/02/28 20:28:38, sra1 wrote: >>> >>>> It would be nice if the place that sets this was obviously the same >>>> >>> calculation. >>> >>>> There are three functions that have the magic offset, expressed in >>>> >>> different >>> >>>> ways (+ sizeof vs [-1]). >>>> >>> >>> I switched Initialize() to also use []. >>> >>> I don't see an easy way to consolidate these all in a single function >>> and still maintain readability, but if you have suggestions I can >>> incorporate them. >>> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h >>> File courgette/memory_allocator.h (right): >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >>> courgette/memory_allocator.h:19: // is deleted when the file handle is >>> closed. >>> On 2011/02/28 20:28:38, sra1 wrote: >>> >>>> Add somewhere: Since the file will be used as backing for a memory >>>> >>> allocation, >>> >>>> it will never be so big that size_t cannot represent it's size. >>>> Otherwise someone will wonder why the size is not [u]int64. >>>> >>> >>> Done. >>> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >>> courgette/memory_allocator.h:71: class TempMapping { >>> On 2011/02/28 20:28:38, sra1 wrote: >>> >>>> The two classes before this are all implementation detail. They could >>>> >>> be moved >>> >>>> to the .cc >>>> >>> >>> This class holds member variables (not pointers) of those classes they >>> need to be there. >>> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >>> courgette/memory_allocator.h:96: // or if a heap allocation fails. >>> On 2011/02/28 20:28:38, sra1 wrote: >>> >>>> I would add a comment about the 'why', something like: >>>> >>> >>> Allocating the memory as a mapping of a temporary file solves the >>>> >>> problem that >>> >>>> there might not be enough physical memory and pagefile to support the >>>> allocation. This can happen because these resources are too small, or >>>> >>> already >>> >>>> committed to other processes. Provided there is enough disk, the >>>> >>> temporary file >>> >>>> acts like a pagefile that other processes can't access. >>>> >>> >>> Done. >>> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >>> courgette/memory_allocator.h:109: // deallocate it. >>> I thought about that kind of approach but went with this approach >>> because it was simpler and didn't depend on other allocations. However, >>> you have a point and I'm happy to oblige. >>> Do you mind that I do that on a separate changelist and likely not in >>> time for the push this week? >>> >>> >>> >>> http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... >>> courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation >>> size that we'll attempt. >>> On 2011/02/28 20:28:38, sra1 wrote: >>> >>>> Say why it is 4MB, rather than, say, 128k or 32MB. >>>> >>> >>> To be honest this is a finger-in-the air estimate based on non >>> scientific observation. Let me gather some numbers and pick something >>> that I can more easily justify. As for 32MB, that's likely too big. I >>> ran tests with 40MB as the cliff and it caused other heap allocations to >>> start failing in low memory situatons. >>> >>> >>> http://codereview.chromium.org/6597038/ >>> >> >> >
LGTM http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.cc File courgette/memory_allocator.cc (right): http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.c... courgette/memory_allocator.cc:120: size += sizeof(this); On 2011/02/28 21:17:41, tommi wrote: > On 2011/02/28 20:28:38, sra1 wrote: > > FYI. The assumption here is that the alignment of pointers (this) is as > strict > > or stricter than the alignment of the element type. This is not always true, > > e.g. __m128 has 16-byte alignment. > > Yes, for large objects the alignment will be broken. However, this is not > currently a problem. Are you OK with me putting down a todo for myself to fix > this? TODO is fine. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h File courgette/memory_allocator.h (right): http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:109: // deallocate it. On 2011/02/28 21:17:41, tommi wrote: > I thought about that kind of approach but went with this approach because it was > simpler and didn't depend on other allocations. However, you have a point and > I'm happy to oblige. > Do you mind that I do that on a separate changelist and likely not in time for > the push this week? Sure. http://codereview.chromium.org/6597038/diff/4017/courgette/memory_allocator.h... courgette/memory_allocator.h:115: // 4MB is the maximum heap allocation size that we'll attempt. On 2011/02/28 21:17:41, tommi wrote: > On 2011/02/28 20:28:38, sra1 wrote: > > Say why it is 4MB, rather than, say, 128k or 32MB. > > To be honest this is a finger-in-the air estimate based on non scientific > observation. Let me gather some numbers and pick something that I can more > easily justify. As for 32MB, that's likely too big. I ran tests with 40MB as > the cliff and it caused other heap allocations to start failing in low memory > situatons. Thanks for measuring. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
