Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
683043002:
PartitionAlloc: Distinguish OOMs where a lot of super pages are not committed (Closed)
DescriptionPartitionAlloc: Distinguish OOMs where a lot of super pages are not committed.
This CL distinguishes
specific OOMs (where a lot of super pages are allocated but not committed) from
actual OOMs (where a lot of physical memory is allocated) in crash reports.
- By crashing in partitionOutOfMemoryWithLotsOfUncommitedPages.
- By crashing at a special address (0x9b) in Windows, because reported stack
traces are not accurate.
This check is enabled on 32-bit environments only.
BUG=421387
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185286
Patch Set 1 : #
Total comments: 2
Patch Set 2 : #
Total comments: 20
Patch Set 3 : Reflect comments. #Patch Set 4 : Update totalSize counters after allocation succeeds. #Patch Set 5 : Take DirectMapped pages into account. #Patch Set 6 : IMMEDIATE_CRASH_WITH_FLAG. #
Total comments: 14
Patch Set 7 : Rebase. #Patch Set 8 : Reflect some comments. #Patch Set 9 : Rebase. #Patch Set 10 : Rebase. #Patch Set 11 : #if CPU(32BIT). Remove IMMEDIATE_CRASH_WITH_FLAG. #
Total comments: 2
Patch Set 12 : #if !CPU(64BIT). reinterpret_cast. #
Total comments: 1
Messages
Total messages: 32 (10 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
hiroshige@chromium.org changed reviewers: + kouhei@chromium.org
Kouhei-san, could you check this CL before sending to cevans@ and tsepez@?
https://codereview.chromium.org/683043002/diff/40001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/40001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfCommittedPages + 1000000000 < (uint64_t)root->totalSizeOfSuperPages) { 1000000000 should be named as "static const size_t kReasonableSizeOfUnusedPages" if (r->totalSizeOfSuperPages - r->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages)
Patchset #2 (id:60001) has been deleted
Thank you for reviewing and suggestion! https://codereview.chromium.org/683043002/diff/40001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/40001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfCommittedPages + 1000000000 < (uint64_t)root->totalSizeOfSuperPages) { On 2014/10/29 09:34:32, kouhei wrote: > 1000000000 should be named as "static const size_t kReasonableSizeOfUnusedPages" > if (r->totalSizeOfSuperPages - r->totalSizeOfCommittedPages > > kReasonableSizeOfUnusedPages) Done.
lgtm
hiroshige@chromium.org changed reviewers: + cevans@chromium.org, tsepez@chromium.org
Thanks! cevans@ and tsepez@, could you take a look? https://codereview.chromium.org/658113002/ fixed a PartitionAlloc problem on 64-bit, but not 32-bit. This CL is intended to make the problem distinguishable by crash stack traces, and check whether/how often the problem still occurs on 32-bit.
Mostly nits https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { why this promotion to unit64_t? Also, are we sure that root->totalSizeOfSuperPages >= root->totalSizeOfCommittedPages https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:306: ((void(*)())0x0000009b)(); Maybe we need IMMEDIATE_CRASH_WITH_FLAG(0x9b) macro along the lines of immediate_crash since there may be platforms where we want to use a builtin. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAlloc.h File Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:187: // If the total size of allocated but not committed pages exceeds this value, nit: size in bytes? pages?
Mostly nits
https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { On 2014/11/06 17:32:51, Tom Sepez wrote: > why this promotion to unit64_t? Also, are we sure that > root->totalSizeOfSuperPages >= root->totalSizeOfCommittedPages I also don't like the look of these casts. Hopefully we can get rid of them. If not, the coding style is wrong: we always use static_cast. I do believe root->totalSizeOfSuperPages >= root->totalSizeOfCommittedPages should be an invariant that holds. Maybe add some ASSERT() at the code locations where we increase root->totalSizeOfCommittedPages to ASSERT it is still <= root->totalSizeOfSuperPages ? https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { I don't think this condition will ever give more accurate results on 64-bit. I propose #if CPU(32_BIT) for this entire if condition. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:306: ((void(*)())0x0000009b)(); I don't like this, it's a bit arbitrary and doesn't lead to an obviously visible distinction in crash logs. I'd prefer we did it this way: partitionOutOfMemoryWithLotsOfUncommitedPages(); With: static NEVER_INLINE void partitionOutOfMemoryWithLotsOfUncommitedPages() { IMMEDIATE_CRASH(); } https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAlloc.h File Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:187: // If the total size of allocated but not committed pages exceeds this value, Nit: this comment doesn't really explain why we'd want to do this. Maybe explain "this is to try and distinguish "out of address space" from "out of physical memory" ? https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:188: // special crash stack trace is generated at |partitionOutOfMemory|. Nit: "a special"? https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:189: static const size_t kReasonableSizeOfUnusedPages = 1000000000; Nit: this looks to be about 1GB. Maybe formulate it as 1024 * 1024 * 1024 for easy engineer readability?
jl@opera.com changed reviewers: + jl@opera.com
https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { On 2014/11/06 20:43:22, Chris Evans wrote: > On 2014/11/06 17:32:51, Tom Sepez wrote: > > why this promotion to unit64_t? Also, are we sure that > > root->totalSizeOfSuperPages >= root->totalSizeOfCommittedPages > > I also don't like the look of these casts. Hopefully we can get rid of them. If > not, the coding style is wrong: we always use static_cast. > > I do believe root->totalSizeOfSuperPages >= root->totalSizeOfCommittedPages > should be an invariant that holds. Maybe add some ASSERT() at the code locations > where we increase root->totalSizeOfCommittedPages to ASSERT it is still <= > root->totalSizeOfSuperPages ? Since we now include direct mapped allocations in totalSizeOfCommittedPages, it can theoretically be bigger than totalSizeOfSuperPages. In theory, we could have *only* direct mapped allocations, in which case totalSizeOfSuperPages==0 and totalSizeOfCommittedPages>0.
Thanks for reviewing and comments! I added ASSERT's and fixing crases (in Patch Set 4 and 5). In Patch Set 4: update the totalSize values after allocation succeeds (and do not update them when failed). In Patch Set 5: take DirectMapped pages into account. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { > Maybe add some ASSERT() at the code locations > where we increase root->totalSizeOfCommittedPages to ASSERT it is still <= > root->totalSizeOfSuperPages ? Added assertions, and removed casts. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { > I don't think this condition will ever give more accurate results on 64-bit. I > propose #if CPU(32_BIT) for this entire if condition. I expect there are no out-of-virtual-address-space crash detected here, but I'd like to enable this check in 64-bit to confirm that. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:301: if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { > Since we now include direct mapped allocations in totalSizeOfCommittedPages, it > can theoretically be bigger than totalSizeOfSuperPages. In theory, we could have > *only* direct mapped allocations, in which case totalSizeOfSuperPages==0 and > totalSizeOfCommittedPages>0. Yes, and I added totalSizeOfDirectMappedPages in Patch Set 5. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:306: ((void(*)())0x0000009b)(); cevans@, Agree, I also prefer what you suggested, but in Windows many functions (partitionOutOfMemoryWithLotsOfUncommitedPages in this case) won't appear in the crash stack traces, even with the NEVER_INLINE directive. I'm not sure why (frame pointer omission?), but in the crash reports, there are no partitionOutOfMemory (or partitionFull) reported on Windows. Does anyone knows how to work around this? https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAlloc.h File Source/wtf/PartitionAlloc.h (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:187: // If the total size of allocated but not committed pages exceeds this value, In bytes. Add it to the comment. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:187: // If the total size of allocated but not committed pages exceeds this value, On 2014/11/06 20:43:22, Chris Evans wrote: > Nit: this comment doesn't really explain why we'd want to do this. Maybe explain > "this is to try and distinguish "out of address space" from "out of physical > memory" ? Done. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:187: // If the total size of allocated but not committed pages exceeds this value, On 2014/11/06 20:43:22, Chris Evans wrote: > Nit: this comment doesn't really explain why we'd want to do this. Maybe explain > "this is to try and distinguish "out of address space" from "out of physical > memory" ? Done. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:188: // special crash stack trace is generated at |partitionOutOfMemory|. On 2014/11/06 20:43:22, Chris Evans wrote: > Nit: "a special"? Done. https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.h:189: static const size_t kReasonableSizeOfUnusedPages = 1000000000; On 2014/11/06 20:43:22, Chris Evans wrote: > Nit: this looks to be about 1GB. Maybe formulate it as 1024 * 1024 * 1024 for > easy engineer readability? Done.
Patchset #5 (id:140001) has been deleted
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/80001/Source/wtf/PartitionAllo... Source/wtf/PartitionAlloc.cpp:306: ((void(*)())0x0000009b)(); Added IMMEDIATE_CRASH_WITH_FLAG and partitionOutOfMemoryWithLotsOfUncommitedPages() in Patch Set 6.
https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:300: Nit: Remove extra newline? https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:305: IMMEDIATE_CRASH_WITH_FLAG(0x9b); Do we think this strange quirk of Windows (missing frames in stack trace) is worth adding a brand new macro for? I'd be happy with #if OS(WIN) .. do the magic bad dereference .. #endif IMMEDIATE_CRASH() Also: does the "missing frames" problem affect 64-bit Windows? If not, we can narrow the special hack to just Windows and just 32-bit which would be nice. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:310: ASSERT(root->totalSizeOfCommittedPages <= root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages); Nit: for clarity, maybe we only need to ASSERT() directly after modifying one of these values. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:311: if (root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages - root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { I still don't think this test has much value in 64-bit. Even if we do have 1GB of uncommitted pages on 64-bit, I think this has zero correlation with running out of virtual address space. A 64-bit address space is enormous, even on ARM64. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:590: root->totalSizeOfCommittedPages += size + kSystemPageSize; Nit: calculate "size + kSystemPageSize" into a separate variable to avoid repetition? https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:647: PartitionRootBase* root = partitionPageToRoot(page); Nit: calculate "page->bucket->slotSize + kSystemPageSize" into a separate variable to avoid repetition?
https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (left): https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:323: root->totalSizeOfCommittedPages += totalSize; Whoa, the changes in this file look like an important bugfix: the accounting is wrong if we subsequently fail the allocation. Feel free to extract out the changes in this function (minus the new ASSERTs) and TBR it to me.
On 2014/11/08 07:07:15, Chris Evans wrote: > https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... > File Source/wtf/PartitionAlloc.cpp (left): > > https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:323: root->totalSizeOfCommittedPages += totalSize; > Whoa, the changes in this file look like an important bugfix: the accounting is > wrong if we subsequently fail the allocation. > > Feel free to extract out the changes in this function (minus the new ASSERTs) > and TBR it to me. I mean "function" not "file" above.
Patchset #11 (id:300001) has been deleted
https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (left): https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:323: root->totalSizeOfCommittedPages += totalSize; On 2014/11/08 07:07:15, Chris Evans wrote: > Whoa, the changes in this file look like an important bugfix: the accounting is > wrong if we subsequently fail the allocation. > > Feel free to extract out the changes in this function (minus the new ASSERTs) > and TBR it to me. Done. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:300: On 2014/11/08 07:04:43, Chris Evans wrote: > Nit: Remove extra newline? Done. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:305: IMMEDIATE_CRASH_WITH_FLAG(0x9b); > Do we think this strange quirk of Windows (missing frames in stack trace) is > worth adding a brand new macro for? > I'd be happy with ... Done. I withdraw the macro because I feel its current implementation (crashing at magic number address) is somehow ugly and not ready for use in general. > Also: does the "missing frames" problem affect 64-bit Windows? I think so, because there are no crashes which includes partitionFull or partitionOutOfMemory in stack traces on Win64. (and there are such crashes on amd64 Linux and Mac) https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:310: ASSERT(root->totalSizeOfCommittedPages <= root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages); On 2014/11/08 07:04:44, Chris Evans wrote: > Nit: for clarity, maybe we only need to ASSERT() directly after modifying one of > these values. Done. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:311: if (root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages - root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) { OK, I added #if CPU(32_BIT) to limit this to 32-bit only. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:590: root->totalSizeOfCommittedPages += size + kSystemPageSize; On 2014/11/08 07:04:44, Chris Evans wrote: > Nit: calculate "size + kSystemPageSize" into a separate variable to avoid > repetition? Done. https://codereview.chromium.org/683043002/diff/200001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:647: PartitionRootBase* root = partitionPageToRoot(page); On 2014/11/08 07:04:44, Chris Evans wrote: > Nit: calculate "page->bucket->slotSize + kSystemPageSize" into a separate > variable to avoid repetition? Done.
LGTM with one nit. https://codereview.chromium.org/683043002/diff/320001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/320001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:309: ((void(*)())0x9b)(); Nit: should probably use reinterpret_cast to comply with coding standards?
Also updated CL description. https://codereview.chromium.org/683043002/diff/320001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/320001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:309: ((void(*)())0x9b)(); On 2014/11/12 23:01:30, Chris Evans wrote: > Nit: should probably use reinterpret_cast to comply with coding standards? Done. https://codereview.chromium.org/683043002/diff/340001/Source/wtf/PartitionAll... File Source/wtf/PartitionAlloc.cpp (right): https://codereview.chromium.org/683043002/diff/340001/Source/wtf/PartitionAll... Source/wtf/PartitionAlloc.cpp:300: #if !CPU(64BIT) Use CPU(64BIT) instead of CPU(32BIT), beause https://codereview.chromium.org/712303003/ was reverted, and CPU(32BIT) is proposed to be removed in https://codereview.chromium.org/717263005/.
On 2014/11/13 06:26:32, hiroshige wrote: > Also updated CL description. > > https://codereview.chromium.org/683043002/diff/320001/Source/wtf/PartitionAll... > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/683043002/diff/320001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:309: ((void(*)())0x9b)(); > On 2014/11/12 23:01:30, Chris Evans wrote: > > Nit: should probably use reinterpret_cast to comply with coding standards? > > Done. > > https://codereview.chromium.org/683043002/diff/340001/Source/wtf/PartitionAll... > File Source/wtf/PartitionAlloc.cpp (right): > > https://codereview.chromium.org/683043002/diff/340001/Source/wtf/PartitionAll... > Source/wtf/PartitionAlloc.cpp:300: #if !CPU(64BIT) > Use CPU(64BIT) instead of CPU(32BIT), beause > https://codereview.chromium.org/712303003/ was reverted, and > CPU(32BIT) is proposed to be removed in > https://codereview.chromium.org/717263005/. Still LGTM I kind of like CPU(32BIT) though. You might want to land this with CPU(32BIT) and we can debate whether to keep it or not with Tom in the other CL.
> I kind of like CPU(32BIT) though. You might want to land this with CPU(32BIT) > and we can debate whether to keep it or not with Tom in the other CL. Then should I re-land CPU(32BIT) fix before landing this? my CL was reverted and CPU(32BIT) of the current head is still wrong. I think Tom's code in PartitionAlloc was also reverted and thus we can land CPU(32BIT) fix without crashing XP. Or land this with !CPU(64BIT) and modify it to CPU(32BIT) later if we decide to leave CPU(32BIT) in the other CL? Which one do you prefer?
> Or land this with !CPU(64BIT) and modify it to CPU(32BIT) later > if we decide to leave CPU(32BIT) in the other CL? I'll do this.
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/683043002/340001
Message was sent while issue was closed.
Committed patchset #12 (id:340001) as 185286 |
