[heap] Fix searching for a node in FreeListCategory

[heap] Fix searching for a node in FreeListCategory

- The bug is that we did not handle end_ properly in SearchForNodeInList.
- We now consistently account for sizes on page level in FreeList, except when
  filtering evacuation candidates (those are accounted for in FreeListCategory)

BUG=chromium:524425
LOG=N

Committed: https://crrev.com/24ba6c947c6a1aae0336f399778764a70cde027b
Cr-Commit-Position: refs/heads/master@{#31307}

[Michael Lippautz, 2015-10-15 12:23:31]

mlippautz@chromium.org changed reviewers:
+ hpayer@chromium.org

[Michael Lippautz, 2015-10-15 12:23:32]

PTAL

https://codereview.chromium.org/1389293005/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1389293005/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:289:
heap()->old_space()->EvictEvacuationCandidatesFromLinearAllocationArea();
Fixed naming here as we do not touch free lists in any way.

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2133
src/heap/spaces.cc:2133: Page::FromAddress(node->address())
This was a bug, as we did not remove the size when removing evacuation candidate
nodes.

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2181
src/heap/spaces.cc:2181: if (cur_node == end()) {
Bug: end() was not handled properly. The bug flushed out when nodes were skipped
(as sizes were too small) and the last node was a match.

[Hannes Payer (out of office), 2015-10-15 12:45:10]

LGTM, nits attached.

https://codereview.chromium.org/1389293005/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1389293005/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:289:
heap()->old_space()->EvictEvacuationCandidatesFromLinearAllocationArea();
On 2015/10/15 12:23:32, Michael Lippautz wrote:
> Fixed naming here as we do not touch free lists in any way.

Acknowledged.

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2131
src/heap/spaces.cc:2131:
Page::FromAddress(node->address())->IsEvacuationCandidate()) {
Let's re-write this piece of code.
Create a local variable page to avoid two times
Page::FromAddress(node->address()).

while (node != nullptr) {
  Page* page = Page::FromAddress(node->address());
  if (!page->IsEvacuationCandidate()) break;
  ...

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2133
src/heap/spaces.cc:2133: Page::FromAddress(node->address())
On 2015/10/15 12:23:32, Michael Lippautz wrote:
> This was a bug, as we did not remove the size when removing evacuation
candidate
> nodes.

Acknowledged.

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2181
src/heap/spaces.cc:2181: if (cur_node == end()) {
On 2015/10/15 12:23:32, Michael Lippautz wrote:
> Bug: end() was not handled properly. The bug flushed out when nodes were
skipped
> (as sizes were too small) and the last node was a match.

Acknowledged.

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2189
src/heap/spaces.cc:2189: page_for_node->add_available_in_free_list(type_,
-size);
I like it that this counter is updated here, since this is the place where the
node is removed from the free-list. We should have a look if we can do the
counter update in all cases right after removing/adding the node. We can do that
in a follow-up cl.

[Michael Lippautz, 2015-10-15 12:53:21]

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2131
src/heap/spaces.cc:2131:
Page::FromAddress(node->address())->IsEvacuationCandidate()) {
On 2015/10/15 12:45:10, Hannes Payer wrote:
> Let's re-write this piece of code.
> Create a local variable page to avoid two times
> Page::FromAddress(node->address()).
> 
> while (node != nullptr) {
>   Page* page = Page::FromAddress(node->address());
>   if (!page->IsEvacuationCandidate()) break;
>   ...

Done.

https://codereview.chromium.org/1389293005/diff/1/src/heap/spaces.cc#newcode2189
src/heap/spaces.cc:2189: page_for_node->add_available_in_free_list(type_,
-size);
On 2015/10/15 12:45:10, Hannes Payer wrote:
> I like it that this counter is updated here, since this is the place where the
> node is removed from the free-list. We should have a look if we can do the
> counter update in all cases right after removing/adding the node. We can do
that
> in a follow-up cl.

Acknowledged.

[Michael Lippautz, 2015-10-15 13:37:42]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-15 13:37:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1389293005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1389293005/40001

[commit-bot: I haz the power, 2015-10-15 14:20:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-15 14:20:07]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2015-10-15 14:47:22]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-10-15 14:47:22]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1389293005/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-15 14:47:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1389293005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1389293005/40001

[commit-bot: I haz the power, 2015-10-15 14:49:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-15 14:49:17]

Patchset 3 (id:??) landed as
https://crrev.com/24ba6c947c6a1aae0336f399778764a70cde027b
Cr-Commit-Position: refs/heads/master@{#31307}