Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2299753002:
Made zone segments aligned in memory and included a pointer to the zone in the header. Larger objec…
DescriptionMade zone segments aligned in memory and included a pointer to the zone in the header. Larger objects get their own segments.
BUG=
Patch Set 1 : Made zone segments aligned in memory and included a pointer to the zone in the header. Larger objec… #
Total comments: 2
Patch Set 2 : Simplified Segment::size() and fixed ::is_big_object_segment() #
Total comments: 3
Patch Set 3 : Reaction to comments #
Total comments: 17
Patch Set 4 : Reaction to comments #
Total comments: 18
Patch Set 5 : Addressing review comments. #
Total comments: 2
Patch Set 6 : Added alignment code in Zone::NewSegment #
Total comments: 5
Patch Set 7 : Fix for windows #
Total comments: 8
Patch Set 8 : Deactivated malfuntioning test and added some zone unittests #Patch Set 9 : Cast list->length() and list->capacity() to size_t #
Total comments: 10
Patch Set 10 : Only zap commited region #Patch Set 11 : Reaction to Review comments #Patch Set 12 : Added a zone segment pool for small segments to avoid frequent sys calls #
Messages
Total messages: 87 (58 generated)
heimbuef@google.com changed reviewers: + jochen@chromium.org
PTAL
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/2299753002/diff/20001/src/zone.h File src/zone.h (right): https://codereview.chromium.org/2299753002/diff/20001/src/zone.h#newcode39 src/zone.h:39: friend class Segment; nit. please move those declarations down to the private: section https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2299753002/diff/40001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/40001/src/zone.cc#newcode255 src/zone.cc:255: v8::base::VirtualMemory::CommitRegion(vm.address(), vm.size(), false); vm.address() is not guaranteed to be aligned, you still need to align upwards yourself. In fact, you may overallocate up with 1MB with this approach per segment https://codereview.chromium.org/2299753002/diff/40001/src/zone.h File src/zone.h (right): https://codereview.chromium.org/2299753002/diff/40001/src/zone.h#newcode96 src/zone.h:96: // Always align new segments to this size nit. comments should be full sentences, so here it should end in a fullstop
another problem I see is that the accounting allocator no longer correctly keeps track of the memory held by zones with this
On 2016/09/01 12:05:50, jochen wrote: > another problem I see is that the accounting allocator no longer correctly keeps > track of the memory held by zones with this You are right, I omitted that when I began and forgot about it later on .. thanks for catching that!
On 2016/09/01 12:12:13, heimbuef wrote: > On 2016/09/01 12:05:50, jochen wrote: > > another problem I see is that the accounting allocator no longer correctly > keeps > > track of the memory held by zones with this > > You are right, I omitted that when I began and forgot about it later on .. > thanks for catching that! PTAL
jochen@chromium.org changed reviewers: + jkummerow@chromium.org
+jkummerow to get more eyes on this. After this change, nothing allocates through AccountingAllocator anymore, right? so it's more like an AllocationAccounter or something now.. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode84 src/zone.cc:84: ASAN_UNPOISON_MEMORY_REGION(vm.address(), vm.size()); that should also be independent of DEBUG https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode86 src/zone.cc:86: memset(vm.address(), kZapDeadByte, vm.size()); nit. do this memsetting whenever ENABLE_HANDLE_ZAPPING is defined, independent of DEBUG https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode116 src/zone.cc:116: }; DISALLOW_COPY_AND_ASSIGN(Segment) https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode145 src/zone.cc:145: // corner case: zero size does this ever happen? malloc(0) would just return a nullptr https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode161 src/zone.cc:161: DCHECK(GetZoneSegmentFromPointer(result)->is_big_object_segment()); dead code ^^^ https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode323 src/zone.cc:323: DCHECK_LE(segment->size(), kSegmentAlignmentSize + 0); why + 0? https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode342 src/zone.cc:342: if (requested > INT_MAX) { why? isn't INT_MAX 2^31 - 1 on Windows, even on 64bit? https://codereview.chromium.org/2299753002/diff/60001/src/zone.h File src/zone.h (right): https://codereview.chromium.org/2299753002/diff/60001/src/zone.h#newcode119 src/zone.h:119: Address NewNormalSegment(size_t size); can you add comments to New{Norma,LargeObject}Segment on how/when to use them? https://codereview.chromium.org/2299753002/diff/60001/src/zone.h#newcode146 src/zone.h:146: Zone* zone() const { return Zone::GetZoneFromPointer(this); } this means you can rip out the zone() methods from all ZoneObjects, right?
On 2016/09/02 11:53:42, jochen wrote: > +jkummerow to get more eyes on this. > > After this change, nothing allocates through AccountingAllocator anymore, right? > > so it's more like an AllocationAccounter or something now.. > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc > File src/zone.cc (right): > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode84 > src/zone.cc:84: ASAN_UNPOISON_MEMORY_REGION(vm.address(), vm.size()); > that should also be independent of DEBUG > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode86 > src/zone.cc:86: memset(vm.address(), kZapDeadByte, vm.size()); > nit. do this memsetting whenever ENABLE_HANDLE_ZAPPING is defined, independent > of DEBUG > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode116 > src/zone.cc:116: }; > DISALLOW_COPY_AND_ASSIGN(Segment) > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode145 > src/zone.cc:145: // corner case: zero size > does this ever happen? malloc(0) would just return a nullptr > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode161 > src/zone.cc:161: > DCHECK(GetZoneSegmentFromPointer(result)->is_big_object_segment()); > dead code ^^^ > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode323 > src/zone.cc:323: DCHECK_LE(segment->size(), kSegmentAlignmentSize + 0); > why + 0? > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode342 > src/zone.cc:342: if (requested > INT_MAX) { > why? isn't INT_MAX 2^31 - 1 on Windows, even on 64bit? > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.h > File src/zone.h (right): > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.h#newcode119 > src/zone.h:119: Address NewNormalSegment(size_t size); > can you add comments to New{Norma,LargeObject}Segment on how/when to use them? > > https://codereview.chromium.org/2299753002/diff/60001/src/zone.h#newcode146 > src/zone.h:146: Zone* zone() const { return Zone::GetZoneFromPointer(this); } > this means you can rip out the zone() methods from all ZoneObjects, right? PTAL
lgtm let's see how this will perform esp. on android where getting virtual address space is expensive.
The CQ bit was checked by heimbuef@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8133)
The CQ bit was checked by heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2299753002/#ps100001 (title: "Fixed Flags")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12058) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/13613)
https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode80 src/zone.cc:80: vm.TakeControl(&virtual_memory_); Why is this dance necessary, as opposed to calling virtual_memory_.Release() directly? https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode147 src/zone.cc:147: // corner case: zero size nit: Comments should have proper capitalization, grammar, and punctuation. (Again below.) https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode148 src/zone.cc:148: if (size == 0) { Does this ever happen? https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode258 src/zone.cc:258: if (vm.IsReserved()) { style nit: I'd reverse the condition to both make it clearer what the regular code path is, and avoid unnecessary indentation, i.e.: if (!vm.IsReserved()) { V8::FatalProcessOutOfMemory("Zone"); return nullptr; } // Continue on this level. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode262 src/zone.cc:262: v8::base::VirtualMemory::CommitRegion(vm.address(), vm.size(), false); This can fail! You must call FatalProcessOutOfMemory() when it returns false. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode283 src/zone.cc:283: // corner case in which a large object segment becomes the head nit: capitalization please https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode287 src/zone.cc:287: // large object segments should be inserted second into the list again, and punctuation https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode307 src/zone.cc:307: // Only normal segments here nit: punctuation, again several times below https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode308 src/zone.cc:308: DCHECK_LE(size, kMaximumSegmentSize + 0); why "+ 0"?
The CQ bit was checked by heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2299753002/#ps140001 (title: "Fixing x86 issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jochen@chromium.org
please wait for approval from Jakob since you changed the CL since my review
https://codereview.chromium.org/2299753002/diff/120001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/120001/src/zone.cc#newcode81 src/zone.cc:81: vm.TakeControl(&virtual_memory_); Question still stands: why do you need |vm| at all, as opposed to working with |virtual_memory_| directly? Since this is not obvious, please add a comment to the source explaining why it's necessary. https://codereview.chromium.org/2299753002/diff/120001/src/zone.cc#newcode314 src/zone.cc:314: DCHECK_LE(size, kMaximumSegmentSize + 0); Question still stands: why "+ 0"?
https://codereview.chromium.org/2299753002/diff/20001/src/zone.h File src/zone.h (right): https://codereview.chromium.org/2299753002/diff/20001/src/zone.h#newcode39 src/zone.h:39: friend class Segment; On 2016/09/01 12:04:52, jochen wrote: > nit. please move those declarations down to the private: section > https://google.github.io/styleguide/cppguide.html#Declaration_Order Acknowledged. https://codereview.chromium.org/2299753002/diff/40001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/40001/src/zone.cc#newcode255 src/zone.cc:255: v8::base::VirtualMemory::CommitRegion(vm.address(), vm.size(), false); On 2016/09/01 12:04:52, jochen wrote: > vm.address() is not guaranteed to be aligned, you still need to align upwards > yourself. > > In fact, you may overallocate up with 1MB with this approach per segment I explicitely checked that with Toon and vm.address() is always aligned. The comment is outdated; I will remove it and write a test for it. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode84 src/zone.cc:84: ASAN_UNPOISON_MEMORY_REGION(vm.address(), vm.size()); On 2016/09/02 11:53:42, jochen wrote: > that should also be independent of DEBUG Done. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode86 src/zone.cc:86: memset(vm.address(), kZapDeadByte, vm.size()); On 2016/09/02 11:53:42, jochen wrote: > nit. do this memsetting whenever ENABLE_HANDLE_ZAPPING is defined, independent > of DEBUG Done. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode116 src/zone.cc:116: }; On 2016/09/02 11:53:42, jochen wrote: > DISALLOW_COPY_AND_ASSIGN(Segment) Done. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode145 src/zone.cc:145: // corner case: zero size On 2016/09/02 11:53:42, jochen wrote: > does this ever happen? malloc(0) would just return a nullptr Yes, quite frequently. I want to guarantee that the result at least points to a valid normal zone segment, so it can be used to get the zone eventually. This could be useful for the ZoneLists. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode161 src/zone.cc:161: DCHECK(GetZoneSegmentFromPointer(result)->is_big_object_segment()); On 2016/09/02 11:53:42, jochen wrote: > dead code ^^^ Done. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode323 src/zone.cc:323: DCHECK_LE(segment->size(), kSegmentAlignmentSize + 0); On 2016/09/02 11:53:42, jochen wrote: > why + 0? The Macro will not compile with a constant, but with an expression it works. https://codereview.chromium.org/2299753002/diff/60001/src/zone.cc#newcode342 src/zone.cc:342: if (requested > INT_MAX) { On 2016/09/02 11:53:42, jochen wrote: > why? isn't INT_MAX 2^31 - 1 on Windows, even on 64bit? Done. https://codereview.chromium.org/2299753002/diff/60001/src/zone.h File src/zone.h (right): https://codereview.chromium.org/2299753002/diff/60001/src/zone.h#newcode146 src/zone.h:146: Zone* zone() const { return Zone::GetZoneFromPointer(this); } On 2016/09/02 11:53:42, jochen wrote: > this means you can rip out the zone() methods from all ZoneObjects, right? We can remove all zone pointers from all ZoneObjects, yes. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode80 src/zone.cc:80: vm.TakeControl(&virtual_memory_); On 2016/09/02 14:11:49, Jakob wrote: > Why is this dance necessary, as opposed to calling virtual_memory_.Release() > directly? Because of the `memset` below. `virtual_memory_` will be zapped by that and we would have no way to release it then. Of course I could move 'the dance' in the conditional area. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode147 src/zone.cc:147: // corner case: zero size On 2016/09/02 14:11:48, Jakob wrote: > nit: Comments should have proper capitalization, grammar, and punctuation. > (Again below.) Acknowledged. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode148 src/zone.cc:148: if (size == 0) { On 2016/09/02 14:11:48, Jakob wrote: > Does this ever happen? Yes, quite frequently. I also plan on using it myself in `ZoneList` to include a `Zone`-pointer in each `ZoneList` even if we do not want to get any capacity just yet. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode258 src/zone.cc:258: if (vm.IsReserved()) { On 2016/09/02 14:11:49, Jakob wrote: > style nit: I'd reverse the condition to both make it clearer what the regular > code path is, and avoid unnecessary indentation, i.e.: > > if (!vm.IsReserved()) { > V8::FatalProcessOutOfMemory("Zone"); > return nullptr; > } > // Continue on this level. Acknowledged. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode262 src/zone.cc:262: v8::base::VirtualMemory::CommitRegion(vm.address(), vm.size(), false); On 2016/09/02 14:11:48, Jakob wrote: > This can fail! You must call FatalProcessOutOfMemory() when it returns false. Acknowledged. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode283 src/zone.cc:283: // corner case in which a large object segment becomes the head On 2016/09/02 14:11:48, Jakob wrote: > nit: capitalization please Acknowledged. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode287 src/zone.cc:287: // large object segments should be inserted second into the list On 2016/09/02 14:11:48, Jakob wrote: > again, and punctuation Acknowledged. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode307 src/zone.cc:307: // Only normal segments here On 2016/09/02 14:11:49, Jakob wrote: > nit: punctuation, again several times below Acknowledged. https://codereview.chromium.org/2299753002/diff/80001/src/zone.cc#newcode308 src/zone.cc:308: DCHECK_LE(size, kMaximumSegmentSize + 0); On 2016/09/02 14:11:48, Jakob wrote: > why "+ 0"? Because the Macro does not compile otherwise. I can include a comment if you want to, but the Macro just seems broken if you ask me. Feel free to investigate.
OK, LGTM.
The CQ bit was checked by heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2299753002/#ps160001 (title: "Added alignment code in Zone::NewSegment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc#newcode267 src/zone.cc:267: auto start = nit: don't use auto, make the type explicit https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc#newcode285 src/zone.cc:285: Segment* result = reinterpret_cast<Segment*>(vm.address()); I think this is line is no longer correct.
https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc#newcode270 src/zone.cc:270: if (start != reinterpret_cast<uintptr_t>(vm.address())) { Actually, can this happen? My reading of base::VirtualMemory is that it guarantees to align its address(). Is that not the case? If so, isn't it a bug in VirtualMemory that should be fixed there rather than worked around here?
The CQ bit was checked by heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2299753002/#ps180001 (title: "Fixed Windows issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:180001) has been deleted
Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc#newcode270 src/zone.cc:270: if (start != reinterpret_cast<uintptr_t>(vm.address())) { Long story short: On Windows, it's not possible to align memory reliable without wasting memory. This cannot be fixed. Changes to how `VirtualMemory` works (to hide that) are not possible either, because too much other stuff is dependent on it. https://codereview.chromium.org/2299753002/diff/160001/src/zone.cc#newcode285 src/zone.cc:285: Segment* result = reinterpret_cast<Segment*>(vm.address()); On 2016/09/05 14:46:27, Jakob Kummerow wrote: > I think this is line is no longer correct. Done.
The CQ bit was checked by heimbuef@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2299753002/#ps200001 (title: "Fix for windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jkummerow@chromium.org
Unaddressed comments -> not lgtm currently. The workflow is: if you get an lg-tm and you have to make further changes, then you request another review. Silently sending random hacks to the CQ is not how we roll. (You can send dry runs to specific bots if needed.) https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode123 src/zone.cc:123: // The beginning of the aligned memory of the segment. This comment does not match the field name. https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode272 src/zone.cc:272: auto base = reinterpret_cast<uintptr_t>(vm.address()) & kSegmentAlignmentMask; don't use auto https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode274 src/zone.cc:274: // On Windows, the address can actually be off. it's not exactly *off*... // On Windows, VirtualMemory can fail to allocate aligned memory. https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode281 src/zone.cc:281: auto end = reinterpret_cast<uintptr_t>(vm.address()) + vm.size(); don't use auto
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12291) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/13845)
Patchset #11 (id:240001) has been deleted
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:220001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #6 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
Patchset #8 (id:260001) has been deleted
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
Patchset #8 (id:320001) has been deleted
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/13855)
PTAL https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode123 src/zone.cc:123: // The beginning of the aligned memory of the segment. On 2016/09/05 16:04:04, Jakob Kummerow wrote: > This comment does not match the field name. Done. https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode272 src/zone.cc:272: auto base = reinterpret_cast<uintptr_t>(vm.address()) & kSegmentAlignmentMask; On 2016/09/05 16:04:04, Jakob Kummerow wrote: > don't use auto Done. https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode274 src/zone.cc:274: // On Windows, the address can actually be off. On 2016/09/05 16:04:04, Jakob Kummerow wrote: > it's not exactly *off*... > > // On Windows, VirtualMemory can fail to allocate aligned memory. Done. https://codereview.chromium.org/2299753002/diff/200001/src/zone.cc#newcode281 src/zone.cc:281: auto end = reinterpret_cast<uintptr_t>(vm.address()) + vm.size(); On 2016/09/05 16:04:04, Jakob Kummerow wrote: > don't use auto Done.
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/13859) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
https://codereview.chromium.org/2299753002/diff/360001/src/zone.cc File src/zone.cc (right): https://codereview.chromium.org/2299753002/diff/360001/src/zone.cc#newcode272 src/zone.cc:272: auto base = Address(reinterpret_cast<uintptr_t>(vm.address()) & Again: please don't use "auto". The style guide says "auto" can be used when doing so increases readability, the canonical example being overly verbose stdlib iterator types. However, "auto base" is not any more readable than "Address base", but instead requires the reader to invest a little more effort to understand what this code does. I insist. https://codereview.chromium.org/2299753002/diff/360001/src/zone.cc#newcode282 src/zone.cc:282: auto end = Address(reinterpret_cast<uintptr_t>(vm.address()) + vm.size()); Same here. https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... File test/unittests/zone-unittest.cc (right): https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:1: // Copyright 2014 the V8 project authors. All rights reserved. nit: 2016 https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:9: #include "src/objects.h" nit: when you #include foo-inl.h, you don't need to #include foo.h. https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:10: nit: no empty line necessary here. Instead, sort #includes alphabetically. https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:12: #include "src/handles.h" again https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:23: data[i] = static_cast<byte>(i % 100); how about s/i % 100/i & 0xFF/ ? https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:34: void empty_alloc() { Any reason this is a separate function? I'd prefer to just inline it into its single callsite for simplicity. https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:49: const size_t kNumberOfChunks = 10000; How long does it take to run these tests? https://codereview.chromium.org/2299753002/diff/360001/test/unittests/zone-un... test/unittests/zone-unittest.cc:114: auto list = new (&zone) ZoneList<byte>(0, &zone); s/auto/ZoneList<byte>/
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by heimbuef@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8422) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
