Revision: 21431
Author: [email protected]
Date: Thu May 22 11:13:37 2014 UTC
Log: Attempt no. 3 to fix Heap::IsHeapIterable and HeapIterator.
Now we remember new space's top pointer after the last GC to find out if
there was a new space allocation since the last GC.
Unfortunately, this not completely safe - the debugger has a callback hook
(that can call to JS) at the end of the GC epilogue that can in theory
allocate and possibly make the heap non-iterable. We can only hope this
does not happen.
BUG=373283
[email protected]
LOG=N
Review URL: https://codereview.chromium.org/291193005
http://code.google.com/p/v8/source/detail?r=21431
Added:
/branches/bleeding_edge/test/mjsunit/regress/regress-373283.js
Modified:
/branches/bleeding_edge/src/debug.cc
/branches/bleeding_edge/src/heap-profiler.cc
/branches/bleeding_edge/src/heap-snapshot-generator.cc
/branches/bleeding_edge/src/heap.cc
/branches/bleeding_edge/src/heap.h
/branches/bleeding_edge/src/liveedit.cc
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/test/cctest/test-api.cc
/branches/bleeding_edge/test/cctest/test-heap.cc
/branches/bleeding_edge/test/cctest/test-object-observe.cc
=======================================
--- /dev/null
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-373283.js Thu May
22 11:13:37 2014 UTC
@@ -0,0 +1,18 @@
+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --deopt-every-n-times=1
+
+function __f_0() {
+ var x = [];
+ x[21] = 1;
+ x[21] + 0;
+}
+
+for (var i = 0; i < 3; i++) __f_0();
+%OptimizeFunctionOnNextCall(__f_0);
+for (var i = 0; i < 10; i++) __f_0();
+%OptimizeFunctionOnNextCall(__f_0);
+__f_0();
+%GetScript("foo");
=======================================
--- /branches/bleeding_edge/src/debug.cc Wed May 21 15:00:32 2014 UTC
+++ /branches/bleeding_edge/src/debug.cc Thu May 22 11:13:37 2014 UTC
@@ -2053,6 +2053,7 @@
Heap* heap = isolate_->heap();
heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
"preparing for breakpoints");
+ HeapIterator iterator(heap);
// Ensure no GC in this scope as we are going to use gc_metadata
// field in the Code object to mark active functions.
@@ -2072,7 +2073,6 @@
// Scan the heap for all non-optimized functions which have no
// debug break slots and are not active or inlined into an active
// function and mark them for lazy compilation.
- HeapIterator iterator(heap);
HeapObject* obj = NULL;
while (((obj = iterator.next()) != NULL)) {
if (obj->IsJSFunction()) {
@@ -2197,9 +2197,7 @@
Handle<SharedFunctionInfo> target;
Heap* heap = isolate_->heap();
while (!done) {
- { // Extra scope for iterator and no-allocation.
- heap->EnsureHeapIsIterable();
- DisallowHeapAllocation no_alloc_during_heap_iteration;
+ { // Extra scope for iterator.
HeapIterator iterator(heap);
for (HeapObject* obj = iterator.next();
obj != NULL; obj = iterator.next()) {
@@ -2529,8 +2527,6 @@
// scripts which are no longer referenced. The second also sweeps
precisely,
// which saves us doing yet another GC to make the heap iterable.
heap->CollectAllGarbage(Heap::kNoGCFlags, "Debug::CreateScriptCache");
- heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
- "Debug::CreateScriptCache");
ASSERT(script_cache_ == NULL);
script_cache_ = new ScriptCache(isolate_);
@@ -2538,7 +2534,6 @@
// Scan heap for Script objects.
int count = 0;
HeapIterator iterator(heap);
- DisallowHeapAllocation no_allocation;
for (HeapObject* obj = iterator.next(); obj != NULL; obj =
iterator.next()) {
if (obj->IsScript() && Script::cast(obj)->HasValidSource()) {
=======================================
--- /branches/bleeding_edge/src/heap-profiler.cc Wed May 21 09:49:18 2014
UTC
+++ /branches/bleeding_edge/src/heap-profiler.cc Thu May 22 11:13:37 2014
UTC
@@ -173,9 +173,6 @@
Handle<HeapObject> HeapProfiler::FindHeapObjectById(SnapshotObjectId id) {
- heap()->CollectAllGarbage(Heap::kMakeHeapIterableMask,
- "HeapProfiler::FindHeapObjectById");
- DisallowHeapAllocation no_allocation;
HeapObject* object = NULL;
HeapIterator iterator(heap(), HeapIterator::kFilterUnreachable);
// Make sure that object with the given id is still reachable.
=======================================
--- /branches/bleeding_edge/src/heap-snapshot-generator.cc Wed May 21
09:49:18 2014 UTC
+++ /branches/bleeding_edge/src/heap-snapshot-generator.cc Thu May 22
11:13:37 2014 UTC
@@ -2569,10 +2569,6 @@
CHECK(!debug_heap->map_space()->was_swept_conservatively());
#endif
- // The following code uses heap iterators, so we want the heap to be
- // stable. It should follow TagGlobalObjects as that can allocate.
- DisallowHeapAllocation no_alloc;
-
#ifdef VERIFY_HEAP
debug_heap->Verify();
#endif
=======================================
--- /branches/bleeding_edge/src/heap.cc Thu May 22 09:36:20 2014 UTC
+++ /branches/bleeding_edge/src/heap.cc Thu May 22 11:13:37 2014 UTC
@@ -699,6 +699,10 @@
ReportStatisticsAfterGC();
#endif // DEBUG
isolate_->debug()->AfterGarbageCollection();
+
+ // Remember the last top pointer so that we can later find out
+ // whether we allocated in new space since the last GC.
+ new_space_top_after_last_gc_ = new_space()->top();
}
@@ -4321,14 +4325,15 @@
bool Heap::IsHeapIterable() {
return (!old_pointer_space()->was_swept_conservatively() &&
- !old_data_space()->was_swept_conservatively());
+ !old_data_space()->was_swept_conservatively() &&
+ new_space_top_after_last_gc_ == new_space()->top());
}
-void Heap::EnsureHeapIsIterable() {
+void Heap::MakeHeapIterable() {
ASSERT(AllowHeapAllocation::IsAllowed());
if (!IsHeapIterable()) {
- CollectAllGarbage(kMakeHeapIterableMask, "Heap::EnsureHeapIsIterable");
+ CollectAllGarbage(kMakeHeapIterableMask, "Heap::MakeHeapIterable");
}
ASSERT(IsHeapIterable());
}
@@ -5208,6 +5213,7 @@
if (!new_space_.SetUp(reserved_semispace_size_, max_semi_space_size_)) {
return false;
}
+ new_space_top_after_last_gc_ = new_space()->top();
// Initialize old pointer space.
old_pointer_space_ =
@@ -5731,7 +5737,9 @@
HeapIterator::HeapIterator(Heap* heap)
- : heap_(heap),
+ : make_heap_iterable_helper_(heap),
+ no_heap_allocation_(),
+ heap_(heap),
filtering_(HeapIterator::kNoFiltering),
filter_(NULL) {
Init();
@@ -5740,7 +5748,9 @@
HeapIterator::HeapIterator(Heap* heap,
HeapIterator::HeapObjectsFiltering filtering)
- : heap_(heap),
+ : make_heap_iterable_helper_(heap),
+ no_heap_allocation_(),
+ heap_(heap),
filtering_(filtering),
filter_(NULL) {
Init();
=======================================
--- /branches/bleeding_edge/src/heap.h Wed May 21 09:49:18 2014 UTC
+++ /branches/bleeding_edge/src/heap.h Thu May 22 11:13:37 2014 UTC
@@ -767,7 +767,7 @@
// Ensure that we have swept all spaces in such a way that we can iterate
// over all objects. May cause a GC.
- void EnsureHeapIsIterable();
+ void MakeHeapIterable();
// Notify the heap that a context has been disposed.
int NotifyContextDisposed();
@@ -1549,6 +1549,7 @@
LargeObjectSpace* lo_space_;
HeapState gc_state_;
int gc_post_processing_depth_;
+ Address new_space_top_after_last_gc_;
// Returns the amount of external memory registered since last global gc.
int64_t PromotedExternalMemorySize();
@@ -2391,6 +2392,9 @@
// aggregates the specific iterators for the different spaces as
// these can only iterate over one space only.
//
+// HeapIterator ensures there is no allocation during its lifetime
+// (using an embedded DisallowHeapAllocation instance).
+//
// HeapIterator can skip free list nodes (that is, de-allocated heap
// objects that still remain in the heap). As implementation of free
// nodes filtering uses GC marks, it can't be used during MS/MC GC
@@ -2413,12 +2417,18 @@
void reset();
private:
+ struct MakeHeapIterableHelper {
+ explicit MakeHeapIterableHelper(Heap* heap) {
heap->MakeHeapIterable(); }
+ };
+
// Perform the initialization.
void Init();
// Perform all necessary shutdown (destruction) work.
void Shutdown();
HeapObject* NextObject();
+ MakeHeapIterableHelper make_heap_iterable_helper_;
+ DisallowHeapAllocation no_heap_allocation_;
Heap* heap_;
HeapObjectsFiltering filtering_;
HeapObjectsFilter* filter_;
=======================================
--- /branches/bleeding_edge/src/liveedit.cc Thu May 22 07:32:59 2014 UTC
+++ /branches/bleeding_edge/src/liveedit.cc Thu May 22 11:13:37 2014 UTC
@@ -939,13 +939,10 @@
// to code objects (that are never in new space) without worrying about
// write barriers.
Heap* heap = original->GetHeap();
- heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
- "liveedit.cc ReplaceCodeObject");
+ HeapIterator iterator(heap);
ASSERT(!heap->InNewSpace(*substitution));
- DisallowHeapAllocation no_allocation;
-
ReplacingVisitor visitor(*original, *substitution);
// Iterate over all roots. Stack frames may have pointer into original
code,
@@ -955,7 +952,6 @@
// Now iterate over all pointers of all objects, including code_target
// implicit pointers.
- HeapIterator iterator(heap);
for (HeapObject* obj = iterator.next(); obj != NULL; obj =
iterator.next()) {
obj->Iterate(&visitor);
}
@@ -982,7 +978,7 @@
// If literal count didn't change, simply go over all functions
// and clear literal arrays.
ClearValuesVisitor visitor;
- IterateJSFunctions(*shared_info, &visitor);
+ IterateJSFunctions(shared_info, &visitor);
} else {
// When literal count changes, we have to create new array instances.
// Since we cannot create instances when iterating heap, we should
first
@@ -1017,16 +1013,14 @@
// Iterates all function instances in the HEAP that refers to the
// provided shared_info.
template<typename Visitor>
- static void IterateJSFunctions(SharedFunctionInfo* shared_info,
+ static void IterateJSFunctions(Handle<SharedFunctionInfo> shared_info,
Visitor* visitor) {
- DisallowHeapAllocation no_allocation;
-
HeapIterator iterator(shared_info->GetHeap());
for (HeapObject* obj = iterator.next(); obj != NULL;
obj = iterator.next()) {
if (obj->IsJSFunction()) {
JSFunction* function = JSFunction::cast(obj);
- if (function->shared() == shared_info) {
+ if (function->shared() == *shared_info) {
visitor->visit(function);
}
}
@@ -1039,13 +1033,13 @@
Handle<SharedFunctionInfo> shared_info, Isolate* isolate) {
CountVisitor count_visitor;
count_visitor.count = 0;
- IterateJSFunctions(*shared_info, &count_visitor);
+ IterateJSFunctions(shared_info, &count_visitor);
int size = count_visitor.count;
Handle<FixedArray> result = isolate->factory()->NewFixedArray(size);
if (size > 0) {
CollectVisitor collect_visitor(result);
- IterateJSFunctions(*shared_info, &collect_visitor);
+ IterateJSFunctions(shared_info, &collect_visitor);
}
return result;
}
@@ -1165,7 +1159,7 @@
Handle<SharedFunctionInfo> shared_info = shared_info_wrapper.GetInfo();
- isolate->heap()->EnsureHeapIsIterable();
+ isolate->heap()->MakeHeapIterable();
if (IsJSFunctionCode(shared_info->code())) {
Handle<Code> code = compile_info_wrapper.GetFunctionCode();
@@ -1402,7 +1396,7 @@
info->set_end_position(new_function_end);
info->set_function_token_position(new_function_token_pos);
- info->GetIsolate()->heap()->EnsureHeapIsIterable();
+ info->GetIsolate()->heap()->MakeHeapIterable();
if (IsJSFunctionCode(info->code())) {
// Patch relocation info section of the code.
@@ -1840,16 +1834,13 @@
Handle<FixedArray> result,
int len) {
Isolate* isolate = shared_info_array->GetIsolate();
- Heap* heap = isolate->heap();
- heap->EnsureHeapIsIterable();
bool found_suspended_activations = false;
ASSERT_LE(len, result->length());
- DisallowHeapAllocation no_allocation;
-
FunctionPatchabilityStatus active = FUNCTION_BLOCKED_ACTIVE_GENERATOR;
+ Heap* heap = isolate->heap();
HeapIterator iterator(heap);
HeapObject* obj = NULL;
while ((obj = iterator.next()) != NULL) {
=======================================
--- /branches/bleeding_edge/src/runtime.cc Wed May 21 12:36:37 2014 UTC
+++ /branches/bleeding_edge/src/runtime.cc Thu May 22 11:13:37 2014 UTC
@@ -13115,14 +13115,6 @@
HandleScope scope(isolate);
ASSERT(args.length() == 3);
- // First perform a full GC in order to avoid references from dead
objects.
- Heap* heap = isolate->heap();
-
heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugReferencedBy");
- // The heap iterator reserves the right to do a GC to make the heap
iterable.
- // Due to the GC above we know it won't need to do that, but it seems
cleaner
- // to get the heap iterator constructed before we start having
unprotected
- // Object* locals that are not protected by handles.
-
// Check parameters.
CONVERT_ARG_HANDLE_CHECKED(JSObject, target, 0);
CONVERT_ARG_HANDLE_CHECKED(Object, instance_filter, 1);
@@ -13140,21 +13132,27 @@
// Get the number of referencing objects.
int count;
- HeapIterator heap_iterator(heap);
- count = DebugReferencedBy(&heap_iterator,
- *target, *instance_filter, max_references,
- NULL, 0, *arguments_function);
+ // First perform a full GC in order to avoid dead objects and to make
the heap
+ // iterable.
+ Heap* heap = isolate->heap();
+
heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugConstructedBy");
+ {
+ HeapIterator heap_iterator(heap);
+ count = DebugReferencedBy(&heap_iterator,
+ *target, *instance_filter, max_references,
+ NULL, 0, *arguments_function);
+ }
// Allocate an array to hold the result.
Handle<FixedArray> instances = isolate->factory()->NewFixedArray(count);
// Fill the referencing objects.
- // AllocateFixedArray above does not make the heap non-iterable.
- ASSERT(heap->IsHeapIterable());
- HeapIterator heap_iterator2(heap);
- count = DebugReferencedBy(&heap_iterator2,
- *target, *instance_filter, max_references,
- *instances, count, *arguments_function);
+ {
+ HeapIterator heap_iterator(heap);
+ count = DebugReferencedBy(&heap_iterator,
+ *target, *instance_filter, max_references,
+ *instances, count, *arguments_function);
+ }
// Return result as JS array.
Handle<JSFunction> constructor(
@@ -13205,9 +13203,6 @@
HandleScope scope(isolate);
ASSERT(args.length() == 2);
- // First perform a full GC in order to avoid dead objects.
- Heap* heap = isolate->heap();
-
heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugConstructedBy");
// Check parameters.
CONVERT_ARG_HANDLE_CHECKED(JSFunction, constructor, 0);
@@ -13216,24 +13211,31 @@
// Get the number of referencing objects.
int count;
- HeapIterator heap_iterator(heap);
- count = DebugConstructedBy(&heap_iterator,
- *constructor,
- max_references,
- NULL,
- 0);
+ // First perform a full GC in order to avoid dead objects and to make
the heap
+ // iterable.
+ Heap* heap = isolate->heap();
+
heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugConstructedBy");
+ {
+ HeapIterator heap_iterator(heap);
+ count = DebugConstructedBy(&heap_iterator,
+ *constructor,
+ max_references,
+ NULL,
+ 0);
+ }
// Allocate an array to hold the result.
Handle<FixedArray> instances = isolate->factory()->NewFixedArray(count);
- ASSERT(heap->IsHeapIterable());
// Fill the referencing objects.
- HeapIterator heap_iterator2(heap);
- count = DebugConstructedBy(&heap_iterator2,
- *constructor,
- max_references,
- *instances,
- count);
+ {
+ HeapIterator heap_iterator2(heap);
+ count = DebugConstructedBy(&heap_iterator2,
+ *constructor,
+ max_references,
+ *instances,
+ count);
+ }
// Return result as JS array.
Handle<JSFunction> array_function(
@@ -13365,8 +13367,6 @@
int number;
Heap* heap = isolate->heap();
{
- heap->EnsureHeapIsIterable();
- DisallowHeapAllocation no_allocation;
HeapIterator heap_iterator(heap);
Script* scr = *script;
FixedArray* arr = *array;
@@ -13374,8 +13374,6 @@
}
if (number > kBufferSize) {
array = isolate->factory()->NewFixedArray(number);
- heap->EnsureHeapIsIterable();
- DisallowHeapAllocation no_allocation;
HeapIterator heap_iterator(heap);
Script* scr = *script;
FixedArray* arr = *array;
@@ -14458,8 +14456,6 @@
Handle<Script> script;
Factory* factory = script_name->GetIsolate()->factory();
Heap* heap = script_name->GetHeap();
- heap->EnsureHeapIsIterable();
- DisallowHeapAllocation no_allocation_during_heap_iteration;
HeapIterator iterator(heap);
HeapObject* obj = NULL;
while (script.is_null() && ((obj = iterator.next()) != NULL)) {
=======================================
--- /branches/bleeding_edge/test/cctest/test-api.cc Thu May 22 08:46:01
2014 UTC
+++ /branches/bleeding_edge/test/cctest/test-api.cc Thu May 22 11:13:37
2014 UTC
@@ -13568,7 +13568,6 @@
static int GetGlobalObjectsCount() {
- CcTest::heap()->EnsureHeapIsIterable();
int count = 0;
i::HeapIterator it(CcTest::heap());
for (i::HeapObject* object = it.next(); object != NULL; object =
it.next())
=======================================
--- /branches/bleeding_edge/test/cctest/test-heap.cc Wed May 21 09:49:18
2014 UTC
+++ /branches/bleeding_edge/test/cctest/test-heap.cc Thu May 22 11:13:37
2014 UTC
@@ -890,7 +890,6 @@
static int ObjectsFoundInHeap(Heap* heap, Handle<Object> objs[], int size)
{
// Count the number of objects found in the heap.
int found_count = 0;
- heap->EnsureHeapIsIterable();
HeapIterator iterator(heap);
for (HeapObject* obj = iterator.next(); obj != NULL; obj =
iterator.next()) {
for (int i = 0; i < size; i++) {
@@ -1629,9 +1628,8 @@
TEST(TestSizeOfObjectsVsHeapIteratorPrecision) {
CcTest::InitializeVM();
- CcTest::heap()->EnsureHeapIsIterable();
- intptr_t size_of_objects_1 = CcTest::heap()->SizeOfObjects();
HeapIterator iterator(CcTest::heap());
+ intptr_t size_of_objects_1 = CcTest::heap()->SizeOfObjects();
intptr_t size_of_objects_2 = 0;
for (HeapObject* obj = iterator.next();
obj != NULL;
=======================================
--- /branches/bleeding_edge/test/cctest/test-object-observe.cc Wed May 21
09:49:18 2014 UTC
+++ /branches/bleeding_edge/test/cctest/test-object-observe.cc Thu May 22
11:13:37 2014 UTC
@@ -616,7 +616,6 @@
static int GetGlobalObjectsCount() {
- CcTest::heap()->EnsureHeapIsIterable();
int count = 0;
i::HeapIterator it(CcTest::heap());
for (i::HeapObject* object = it.next(); object != NULL; object =
it.next())
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
