Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2314783002:
[heap] Removes spaces.h include from heap.h (Closed)
|
Created:
4 years, 3 months ago by Michael Lippautz Modified:
4 years, 3 months ago Reviewers:
ulan CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan, Michael Starzinger Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Removes spaces.h include from heap.h
Together with the presubmit rule of prohibiting src/heap/* includes except for
heap.h this now properly hides all heap internals.
R=ulan@chromium.org
BUG=
Committed: https://crrev.com/2e3165029d4f8d17f909d1afa4d1b06a3445650d
Cr-Commit-Position: refs/heads/master@{#39211}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : No more spaces.h #
Total comments: 2
Patch Set 4 : Move data structures used in debug mode #
Messages
Total messages: 28 (16 generated)
ptal After this CL the only reason to keep the spaces.h include is that we embed NewSpace into Heap. Will do this separately as it might impact performance.
lgtm
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org Link to the patchset: https://codereview.chromium.org/2314783002/#ps20001 (title: "Rebase")
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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12218) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12217) 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_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12154)
+mstarzinger: FYI Had to add the NewSpace change to make it compile (cyclic dependency). Let's see if we regress anything with the additional indirection. (The InNewSpace checks are manually inlined so that they are still fast.)
The CQ bit was checked by mlippautz@chromium.org 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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12221) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12220)
Description was changed from ========== [heap] Move AllocationResult to heap.h R=ulan@chromium.org BUG= ========== to ========== [heap] Removes spaces.h include from heap.h Together with the presubmit rule of prohibiting src/heap/* includes except for heap.h this now properly hides all heap internals. R=ulan@chromium.org BUG= ==========
Nice! I love it. https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc#newcod... src/heap/heap.cc:5357: if (new_space_ == nullptr) return false; nit: Can this actually ever happen? Shouldn't this abort execution instead of returning a {nullptr} here?
The CQ bit was checked by mlippautz@chromium.org 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...
https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc#newcod... src/heap/heap.cc:5357: if (new_space_ == nullptr) return false; On 2016/09/06 13:51:02, Michael Starzinger wrote: > nit: Can this actually ever happen? Shouldn't this abort execution instead of > returning a {nullptr} here? It should never happen and we do abort. The caller handles this case: if (!heap_.SetUp()) { V8::FatalProcessOutOfMemory("heap setup"); return false; } (Not sure about why there's another return false there ;))
Description was changed from ========== [heap] Removes spaces.h include from heap.h Together with the presubmit rule of prohibiting src/heap/* includes except for heap.h this now properly hides all heap internals. R=ulan@chromium.org BUG= ========== to ========== [heap] Removes spaces.h include from heap.h Together with the presubmit rule of prohibiting src/heap/* includes except for heap.h this now properly hides all heap internals. R=ulan@chromium.org BUG= ==========
On 2016/09/06 13:55:41, Michael Lippautz wrote: > https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc > File src/heap/heap.cc (right): > > https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc#newcod... > src/heap/heap.cc:5357: if (new_space_ == nullptr) return false; > On 2016/09/06 13:51:02, Michael Starzinger wrote: > > nit: Can this actually ever happen? Shouldn't this abort execution instead of > > returning a {nullptr} here? > > It should never happen and we do abort. The caller handles this case: > if (!heap_.SetUp()) { > V8::FatalProcessOutOfMemory("heap setup"); > return false; > } > > (Not sure about why there's another return false there ;)) ftr: You are right, the C++ constructor will never return null but fail with std::bad_alloc. Will fix this (and its other occurrences in heap) in a follow up.
On 2016/09/06 13:55:41, Michael Lippautz wrote: > https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc > File src/heap/heap.cc (right): > > https://codereview.chromium.org/2314783002/diff/40001/src/heap/heap.cc#newcod... > src/heap/heap.cc:5357: if (new_space_ == nullptr) return false; > On 2016/09/06 13:51:02, Michael Starzinger wrote: > > nit: Can this actually ever happen? Shouldn't this abort execution instead of > > returning a {nullptr} here? > > It should never happen and we do abort. The caller handles this case: > if (!heap_.SetUp()) { > V8::FatalProcessOutOfMemory("heap setup"); > return false; > } > > (Not sure about why there's another return false there ;)) ftr: You are right, the C++ constructor will never return null but fail with std::bad_alloc. Will fix this (and its other occurrences in heap) in a follow up.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org Link to the patchset: https://codereview.chromium.org/2314783002/#ps60001 (title: "Move data structures used in debug mode")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Removes spaces.h include from heap.h Together with the presubmit rule of prohibiting src/heap/* includes except for heap.h this now properly hides all heap internals. R=ulan@chromium.org BUG= ========== to ========== [heap] Removes spaces.h include from heap.h Together with the presubmit rule of prohibiting src/heap/* includes except for heap.h this now properly hides all heap internals. R=ulan@chromium.org BUG= Committed: https://crrev.com/2e3165029d4f8d17f909d1afa4d1b06a3445650d Cr-Commit-Position: refs/heads/master@{#39211} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2e3165029d4f8d17f909d1afa4d1b06a3445650d Cr-Commit-Position: refs/heads/master@{#39211} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
