New issue
Advanced search Search tips

Issue 709075 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 694255



Sign in to add a comment

Unify static heap visitors with the ObjectVisitor in V8

Project Member Reported by u...@chromium.org, Apr 6 2017

Issue description

We have a lot of code duplication in object visiting logic due to two ways of defining visitors.

We can use CRTP to make ObjectVisitor fast and remove the static visitors.

 
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/039617d75483e10ded4d60d08d684bba08d8bdb9

commit 039617d75483e10ded4d60d08d684bba08d8bdb9
Author: ulan <ulan@chromium.org>
Date: Fri Apr 07 12:24:21 2017

Handle ExternalStrings directly in the serializer without ObjectVisitor.

The serializer already has code that special cases for some external
strings. We can handle all external strings in one place instead of
splitting the logic between the serializer and the object visitor.

The main benefit is that we remove two virtual functions from the
ObjectVisitor and thus simplify it for all other users.

BUG= chromium:709075 

Review-Url: https://codereview.chromium.org/2799943002
Cr-Commit-Position: refs/heads/master@{#44485}

[modify] https://crrev.com/039617d75483e10ded4d60d08d684bba08d8bdb9/src/heap/mark-compact.cc
[modify] https://crrev.com/039617d75483e10ded4d60d08d684bba08d8bdb9/src/objects-body-descriptors-inl.h
[modify] https://crrev.com/039617d75483e10ded4d60d08d684bba08d8bdb9/src/objects.h
[modify] https://crrev.com/039617d75483e10ded4d60d08d684bba08d8bdb9/src/snapshot/serializer.cc
[modify] https://crrev.com/039617d75483e10ded4d60d08d684bba08d8bdb9/src/snapshot/serializer.h

Comment 3 by u...@chromium.org, Apr 7 2017

Blocking: 694255
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e671ed361026d01342d6032c3dab84abe48d19a6

commit e671ed361026d01342d6032c3dab84abe48d19a6
Author: ulan <ulan@chromium.org>
Date: Tue Apr 25 13:32:18 2017

Decouple root visitors from object visitors.

This patch adds a new interface called RootVisitor and changes the root
iteration functions to accept a RootVisitor instead of an ObjectVisitor.

Future CLs will change ObjectVisitor to provide the host object to all
visiting functions, which will bring it in sync with static visitors.

Having separate visitors for roots and objects removes ambiguity in
VisitPointers and reduces chances of forgetting to record slots.

This is intended as pure refactoring. All places that require behavior
change are marked with TODO and will addressed in future CLs.

BUG= chromium:709075 

Review-Url: https://codereview.chromium.org/2801073006
Cr-Commit-Position: refs/heads/master@{#44852}

[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/BUILD.gn
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/api-arguments.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/api.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/api.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/bootstrapper.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/bootstrapper.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/builtins/builtins-api.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/builtins/builtins.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/builtins/builtins.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/compilation-cache.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/compilation-cache.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/debug/debug.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/debug/debug.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/frames.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/frames.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/global-handles.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/global-handles.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/heap-inl.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/heap.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/heap.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/incremental-marking.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/mark-compact.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/mark-compact.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/object-stats.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/scavenger.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/heap/scavenger.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/interpreter/interpreter.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/interpreter/interpreter.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/isolate.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/isolate.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/objects.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/objects.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/profiler/heap-snapshot-generator.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/profiler/heap-snapshot-generator.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/code-serializer.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/deserializer.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/deserializer.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/partial-serializer.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/serializer-common.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/serializer-common.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/serializer.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/serializer.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/startup-serializer.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/snapshot/startup-serializer.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/v8.gyp
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/v8threads.cc
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/v8threads.h
[add] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/visitors.cc
[add] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/src/visitors.h
[modify] https://crrev.com/e671ed361026d01342d6032c3dab84abe48d19a6/test/cctest/heap/test-heap.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c59f78f611280240d65ab74a8874ce51161bfcd2

commit c59f78f611280240d65ab74a8874ce51161bfcd2
Author: ulan <ulan@chromium.org>
Date: Tue Apr 25 14:19:00 2017

Add a host parameter to ObjectVisitor methods.

This makes an ObjectVisitor as powerful as a StaticVisitor and allows
slots recording in ObjectVisitor.

This patch also renames VisitCell method of ObjectVisitor to
VisitCellPointer, so that VisitCell is free to be used for actually
visiting a cell.

BUG= chromium:709075 

Review-Url: https://codereview.chromium.org/2810653002
Cr-Commit-Position: refs/heads/master@{#44860}

[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/arm/assembler-arm-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/arm64/assembler-arm64-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/concurrent-marking.cc
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/heap-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/heap.cc
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/heap.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/mark-compact-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/mark-compact.cc
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/mark-compact.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/scavenger.cc
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/heap/scavenger.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/ia32/assembler-ia32-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/mips/assembler-mips-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/mips64/assembler-mips64-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/objects-body-descriptors-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/objects.cc
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/objects.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/ppc/assembler-ppc-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/profiler/heap-snapshot-generator.cc
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/s390/assembler-s390-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/snapshot/serializer.cc
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/snapshot/serializer.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/visitors.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/x64/assembler-x64-inl.h
[modify] https://crrev.com/c59f78f611280240d65ab74a8874ce51161bfcd2/src/x87/assembler-x87-inl.h

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 27 2017

Comment 7 by u...@chromium.org, Jul 11 2017

Cc: u...@chromium.org
Owner: mlippautz@chromium.org
Status: Fixed (was: Assigned)
Michael removed static visitors.

Sign in to add a comment