Revert of Additional HandleScopes to limit Handle consumption. (patchset v8#4 id:50001 of https://codereview.chromium.org/1185633002/)

arv · Commit bot · commit 87afca31adb0 · 2015-06-19T18:25:31.000Z
Reason for revert: Fails the following test handle-count-ast handle-count-runtime-... on V8 Linux - nosnap - debug - 1 http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20debug%20-%201/builds/851/steps/Check/logs/stdio Original issue's description: > Additional HandleScopes to limit Handle consumption. > > erikcorry@chromium.org suggested digging into v8 handle usage. Found potential scopes in ast.cc and runtime-literals.cc and added tests. > > The runtime-literals.cc change reduces peak handles in imaging-darkroom.js from 1,282,610 to 428,218. The ast.cc change reduces the peak handles in string-tagcloud.js from 80,738 to 8,176. > > No significant handle count issues found with major websites, but substantial savings on some benchmarks and demos: > > Kraken's imaging-darkroom.js down from 1,282,610 to 428,218 due to runtime-literals.cc scope. > SunSpider's string-tagcloud.js down from 80,738 to 8.176 due to ast.cc > > http://www.flohofwoe.net/demos/dragons_asmjs.html (738,906 -> 478,296) > http://www.flohofwoe.net/demos/instancing_asmjs.html (737,884 -> 477,274) > https://dl.dropboxusercontent.com/u/16662598/Ports/DOSBox-web/doom.html?engine=dosbox-growth.js (1,724,114 -> 1,087,408) > https://kripken.github.io/ammo.js/examples/new/ammo.html (175,784 -> 142,058) > > BUG= > > Committed: https://crrev.com/3a4c7538839186aa38910c66c986abb563f4ccd2 > Cr-Commit-Position: refs/heads/master@{#29155} TBR=yangguo@chromium.org,erikcorry@chromium.org,oth@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review URL: https://codereview.chromium.org/1194873004 Cr-Commit-Position: refs/heads/master@{#29160}
diff --git a/src/ast.cc b/src/ast.cc
@@ -521,10 +521,8 @@ void ArrayLiteral::BuildConstantElements(Isolate* isolate) {
         depth_acc = m_literal->depth() + 1;
       }
     }
-
-    // New handle scope here, needs to be after BuildContants().
-    HandleScope scope(isolate);
     Handle<Object> boilerplate_value = GetBoilerplateValue(element, isolate);
+
     if (boilerplate_value->IsTheHole()) {
       is_holey = true;
       continue;
diff --git a/src/handles-inl.h b/src/handles-inl.h
@@ -92,19 +92,7 @@ HandleScope::HandleScope(Isolate* isolate) {
 
 
 HandleScope::~HandleScope() {
-#ifdef DEBUG
-  if (FLAG_check_handle_count) {
-    int before = NumberOfHandles(isolate_);
-    CloseScope(isolate_, prev_next_, prev_limit_);
-    int after = NumberOfHandles(isolate_);
-    DCHECK(after - before < kCheckHandleThreshold);
-    DCHECK(before < kCheckHandleThreshold);
-  } else {
-#endif  // DEBUG
-    CloseScope(isolate_, prev_next_, prev_limit_);
-#ifdef DEBUG
-  }
-#endif  // DEBUG
+  CloseScope(isolate_, prev_next_, prev_limit_);
 }
 
 
diff --git a/src/handles.h b/src/handles.h
@@ -208,8 +208,6 @@ class HandleScope {
 
   Isolate* isolate() { return isolate_; }
 
-  static const int kCheckHandleThreshold = 16 * 1024;
-
  private:
   // Prevent heap allocation or illegal handle scopes.
   HandleScope(const HandleScope&);
diff --git a/src/heap/heap.cc b/src/heap/heap.cc
@@ -646,8 +646,8 @@ void Heap::GarbageCollectionEpilogue() {
   if (FLAG_print_handles) PrintHandles();
   if (FLAG_gc_verbose) Print();
   if (FLAG_code_stats) ReportCodeStatistics("After GC");
-  if (FLAG_check_handle_count) CheckHandleCount();
 #endif
+  if (FLAG_check_handle_count) CheckHandleCount();
   if (FLAG_deopt_every_n_garbage_collections > 0) {
     // TODO(jkummerow/ulan/jarin): This is not safe! We can't assume that
     // the topmost optimized frame can be deoptimized safely, because it
@@ -5957,9 +5957,7 @@ void Heap::PrintHandles() {
 class CheckHandleCountVisitor : public ObjectVisitor {
  public:
   CheckHandleCountVisitor() : handle_count_(0) {}
-  ~CheckHandleCountVisitor() {
-    CHECK(handle_count_ < HandleScope::kCheckHandleThreshold);
-  }
+  ~CheckHandleCountVisitor() { CHECK(handle_count_ < 2000); }
   void VisitPointers(Object** start, Object** end) {
     handle_count_ += end - start;
   }
diff --git a/src/runtime/runtime-literals.cc b/src/runtime/runtime-literals.cc
@@ -191,7 +191,6 @@ MaybeHandle<Object> Runtime::CreateArrayLiteralBoilerplate(
           isolate->factory()->CopyFixedArray(fixed_array_values);
       copied_elements_values = fixed_array_values_copy;
       for (int i = 0; i < fixed_array_values->length(); i++) {
-        HandleScope scope(isolate);
         if (fixed_array_values->get(i)->IsFixedArray()) {
           // The value contains the constant_properties of a
           // simple object or array literal.
diff --git a/test/cctest/test-types.cc b/test/cctest/test-types.cc
@@ -1505,7 +1505,6 @@ struct Tests : Rep {
   void Union3() {
     // Monotonicity: T1->Is(T2) or T1->Is(T3) implies T1->Is(Union(T2, T3))
     for (TypeIterator it1 = T.types.begin(); it1 != T.types.end(); ++it1) {
-      HandleScope scope(isolate);
       for (TypeIterator it2 = T.types.begin(); it2 != T.types.end(); ++it2) {
         for (TypeIterator it3 = it2; it3 != T.types.end(); ++it3) {
           TypeHandle type1 = *it1;
@@ -1758,7 +1757,6 @@ struct Tests : Rep {
 
     // Monotonicity: T1->Is(T2) and T1->Is(T3) implies T1->Is(Intersect(T2, T3))
     for (TypeIterator it1 = T.types.begin(); it1 != T.types.end(); ++it1) {
-      HandleScope scope(isolate);
       for (TypeIterator it2 = T.types.begin(); it2 != T.types.end(); ++it2) {
         for (TypeIterator it3 = T.types.begin(); it3 != T.types.end(); ++it3) {
           TypeHandle type1 = *it1;
diff --git a/test/mjsunit/handle-count-ast.js b/test/mjsunit/handle-count-ast.js
diff --git a/test/mjsunit/handle-count-runtime-literals.js b/test/mjsunit/handle-count-runtime-literals.js

Original file line number	Diff line number	Diff line change
`@@ -521,10 +521,8 @@ void ArrayLiteral::BuildConstantElements(Isolate* isolate) {`
`521`	`521`	`depth_acc = m_literal->depth() + 1;`
`522`	`522`	`}`
`523`	`523`	`}`
`524`		`-`
`525`		`- // New handle scope here, needs to be after BuildContants().`
`526`		`- HandleScope scope(isolate);`
`527`	`524`	`Handle<Object> boilerplate_value = GetBoilerplateValue(element, isolate);`
	`525`	`+`
`528`	`526`	`if (boilerplate_value->IsTheHole()) {`
`529`	`527`	`is_holey = true;`
`530`	`528`	`continue;`