Heap::gc_count_, last_gc_count, and kGCsBetweenCleanup should be unsigned...

src/heap.cc

 // Copyright 2010 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 115 matching lines @@
 
 // Will be 4 * reserved_semispace_size_ to ensure that young
 // generation can be aligned to its size.
 int Heap::survived_since_last_expansion_ = 0;
 intptr_t Heap::external_allocation_limit_ = 0;
 
 Heap::HeapState Heap::gc_state_ = NOT_IN_GC;
 
 int Heap::mc_count_ = 0;
 int Heap::ms_count_ = 0;
-int Heap::gc_count_ = 0;
+unsigned int Heap::gc_count_ = 0;
 
 GCTracer* Heap::tracer_ = NULL;
 
 int Heap::unflattened_strings_length_ = 0;
 
 int Heap::always_allocate_scope_depth_ = 0;
 int Heap::linear_allocation_scope_depth_ = 0;
 int Heap::contexts_disposed_ = 0;
 
 int Heap::young_survivors_after_last_gc_ = 0;
@@ skipping 3604 matching lines @@
   Struct::cast(result)->InitializeBody(size);
   return result;
 }
 
 
 bool Heap::IdleNotification() {
   static const int kIdlesBeforeScavenge = 4;
   static const int kIdlesBeforeMarkSweep = 7;
   static const int kIdlesBeforeMarkCompact = 8;
   static const int kMaxIdleCount = kIdlesBeforeMarkCompact + 1;
-  static const int kGCsBetweenCleanup = 4;
+  static const unsigned int kGCsBetweenCleanup = 4;
   static int number_idle_notifications = 0;
-  static int last_gc_count = gc_count_;
+  static unsigned int last_gc_count = gc_count_;

[Vyacheslav Egorov (Chromium), 2011/02/21 15:35:14]

There is a bug here: last_gc_count will be initialized once (on the first
invocation of IdleNotification) with gc_count_ value and will not be
reinitialized again so it will always remain the same.

Please split this into declaration and assignment.

And thanks for making me notice this place.

 
   bool uncommit = true;
   bool finished = false;
 
   // Reset the number of idle notifications received when a number of
   // GCs have taken place. This allows another round of cleanup based
   // on idle notifications if enough work has been carried out to
   // provoke a number of garbage collections.
-  if (gc_count_ < last_gc_count + kGCsBetweenCleanup) {
+  if (gc_count_ - last_gc_count < kGCsBetweenCleanup) {
     number_idle_notifications =
         Min(number_idle_notifications + 1, kMaxIdleCount);
   } else {
     number_idle_notifications = 0;
     last_gc_count = gc_count_;
   }
 
   if (number_idle_notifications == kIdlesBeforeScavenge) {
     if (contexts_disposed_ > 0) {
       HistogramTimerScope scope(&Counters::gc_context);
@@ skipping 1665 matching lines @@
 void ExternalStringTable::TearDown() {
   new_space_strings_.Free();
   old_space_strings_.Free();
 }
 
 
 List<Object*> ExternalStringTable::new_space_strings_;
 List<Object*> ExternalStringTable::old_space_strings_;
 
 } }  // namespace v8::internal