From: yurys@chromium.org <yurys@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Tue, 25 Jun 2013 10:09:19 +0000 (+0000)
Subject: Get rid of Isolate::safe_stack_iterator_counter
X-Git-Tag: upstream/4.7.83~13691
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=c846dabcb0b677623c5ad3c7fa5efbd6e30a8fb4;p=platform%2Fupstream%2Fv8.git

Get rid of Isolate::safe_stack_iterator_counter

This change removes per-isolate counter of active SafeStackFrameIterators. The counter is used by stack frames implementations to avoid accessing pointers to heap objects when traversing stack for CPU profiler (so called "safe" mode). Each StackFrame instance is owned by single iterator and has a pointer to it so we can simply mark the iterator as "safe" or not and read the field in the stack frames instead of going into the isolate.

BUG=None
R=loislo@chromium.org, svenpanne@chromium.org

Review URL: https://codereview.chromium.org/17585008

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@15317 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---

diff --git a/src/frames.cc b/src/frames.cc
index 3cf02f4..80fe469 100644
--- a/src/frames.cc
+++ b/src/frames.cc
@@ -93,14 +93,16 @@ StackFrameIterator::StackFrameIterator(Isolate* isolate)
       STACK_FRAME_TYPE_LIST(INITIALIZE_SINGLETON)
       frame_(NULL), handler_(NULL),
       thread_(isolate_->thread_local_top()),
-      fp_(NULL), sp_(NULL), advance_(&StackFrameIterator::AdvanceWithHandler) {
+      fp_(NULL), sp_(NULL), advance_(&StackFrameIterator::AdvanceWithHandler),
+      can_access_heap_objects_(true) {
   Reset();
 }
 StackFrameIterator::StackFrameIterator(Isolate* isolate, ThreadLocalTop* t)
     : isolate_(isolate),
       STACK_FRAME_TYPE_LIST(INITIALIZE_SINGLETON)
       frame_(NULL), handler_(NULL), thread_(t),
-      fp_(NULL), sp_(NULL), advance_(&StackFrameIterator::AdvanceWithHandler) {
+      fp_(NULL), sp_(NULL), advance_(&StackFrameIterator::AdvanceWithHandler),
+      can_access_heap_objects_(true) {
   Reset();
 }
 StackFrameIterator::StackFrameIterator(Isolate* isolate,
@@ -111,7 +113,8 @@ StackFrameIterator::StackFrameIterator(Isolate* isolate,
       thread_(use_top ? isolate_->thread_local_top() : NULL),
       fp_(use_top ? NULL : fp), sp_(sp),
       advance_(use_top ? &StackFrameIterator::AdvanceWithHandler :
-               &StackFrameIterator::AdvanceWithoutHandler) {
+               &StackFrameIterator::AdvanceWithoutHandler),
+      can_access_heap_objects_(false) {
   if (use_top || fp != NULL) {
     Reset();
   }
@@ -166,7 +169,7 @@ void StackFrameIterator::Reset() {
     state.sp = sp_;
     state.pc_address = ResolveReturnAddressLocation(
         reinterpret_cast<Address*>(StandardFrame::ComputePCAddress(fp_)));
-    type = StackFrame::ComputeType(isolate(), &state);
+    type = StackFrame::ComputeType(this, &state);
   }
   if (SingletonFor(type) == NULL) return;
   frame_ = SingletonFor(type, &state);
@@ -268,24 +271,9 @@ bool SafeStackFrameIterator::ExitFrameValidator::IsValidFP(Address fp) {
 }
 
 
-SafeStackFrameIterator::ActiveCountMaintainer::ActiveCountMaintainer(
-    Isolate* isolate)
-    : isolate_(isolate) {
-  isolate_->set_safe_stack_iterator_counter(
-      isolate_->safe_stack_iterator_counter() + 1);
-}
-
-
-SafeStackFrameIterator::ActiveCountMaintainer::~ActiveCountMaintainer() {
-  isolate_->set_safe_stack_iterator_counter(
-      isolate_->safe_stack_iterator_counter() - 1);
-}
-
-
 SafeStackFrameIterator::SafeStackFrameIterator(
     Isolate* isolate,
     Address fp, Address sp, Address low_bound, Address high_bound) :
-    maintainer_(isolate),
     stack_validator_(low_bound, high_bound),
     is_valid_top_(IsValidTop(isolate, low_bound, high_bound)),
     is_valid_fp_(IsWithinBounds(low_bound, high_bound, fp)),
@@ -294,10 +282,6 @@ SafeStackFrameIterator::SafeStackFrameIterator(
   if (!done()) Advance();
 }
 
-bool SafeStackFrameIterator::is_active(Isolate* isolate) {
-  return isolate->safe_stack_iterator_counter() > 0;
-}
-
 
 bool SafeStackFrameIterator::IsValidTop(Isolate* isolate,
                                         Address low_bound, Address high_bound) {
@@ -435,7 +419,8 @@ void StackFrame::SetReturnAddressLocationResolver(
 }
 
 
-StackFrame::Type StackFrame::ComputeType(Isolate* isolate, State* state) {
+StackFrame::Type StackFrame::ComputeType(const StackFrameIterator* iterator,
+                                         State* state) {
   ASSERT(state->fp != NULL);
   if (StandardFrame::IsArgumentsAdaptorFrame(state->fp)) {
     return ARGUMENTS_ADAPTOR;
@@ -450,8 +435,9 @@ StackFrame::Type StackFrame::ComputeType(Isolate* isolate, State* state) {
     // frames as normal JavaScript frames to avoid having to look
     // into the heap to determine the state. This is safe as long
     // as nobody tries to GC...
-    if (SafeStackFrameIterator::is_active(isolate)) return JAVA_SCRIPT;
-    Code::Kind kind = GetContainingCode(isolate, *(state->pc_address))->kind();
+    if (!iterator->can_access_heap_objects_) return JAVA_SCRIPT;
+    Code::Kind kind = GetContainingCode(iterator->isolate(),
+                                        *(state->pc_address))->kind();
     ASSERT(kind == Code::FUNCTION || kind == Code::OPTIMIZED_FUNCTION);
     return (kind == Code::OPTIMIZED_FUNCTION) ? OPTIMIZED : JAVA_SCRIPT;
   }
@@ -459,10 +445,16 @@ StackFrame::Type StackFrame::ComputeType(Isolate* isolate, State* state) {
 }
 
 
+#ifdef DEBUG
+bool StackFrame::can_access_heap_objects() const {
+  return iterator_->can_access_heap_objects_;
+}
+#endif
+
 
 StackFrame::Type StackFrame::GetCallerState(State* state) const {
   ComputeCallerState(state);
-  return ComputeType(isolate(), state);
+  return ComputeType(iterator_, state);
 }
 
 
@@ -622,7 +614,7 @@ bool StandardFrame::IsExpressionInsideHandler(int n) const {
 void StandardFrame::IterateCompiledFrame(ObjectVisitor* v) const {
   // Make sure that we're not doing "safe" stack frame iteration. We cannot
   // possibly find pointers in optimized frames in that state.
-  ASSERT(!SafeStackFrameIterator::is_active(isolate()));
+  ASSERT(can_access_heap_objects());
 
   // Compute the safepoint information.
   unsigned stack_slots = 0;
@@ -754,7 +746,7 @@ Code* JavaScriptFrame::unchecked_code() const {
 
 
 int JavaScriptFrame::GetNumberOfIncomingArguments() const {
-  ASSERT(!SafeStackFrameIterator::is_active(isolate()) &&
+  ASSERT(can_access_heap_objects() &&
          isolate()->heap()->gc_state() == Heap::NOT_IN_GC);
 
   JSFunction* function = JSFunction::cast(this->function());
diff --git a/src/frames.h b/src/frames.h
index 1a78d09..4d4b7ea 100644
--- a/src/frames.h
+++ b/src/frames.h
@@ -321,7 +321,11 @@ class StackFrame BASE_EMBEDDED {
   inline StackHandler* top_handler() const;
 
   // Compute the stack frame type for the given state.
-  static Type ComputeType(Isolate* isolate, State* state);
+  static Type ComputeType(const StackFrameIterator* iterator, State* state);
+
+#ifdef DEBUG
+  bool can_access_heap_objects() const;
+#endif
 
  private:
   const StackFrameIterator* iterator_;
@@ -782,11 +786,6 @@ class StackFrameIterator BASE_EMBEDDED {
   // An iterator that iterates over a given thread's stack.
   StackFrameIterator(Isolate* isolate, ThreadLocalTop* t);
 
-  // An iterator that can start from a given FP address.
-  // If use_top, then work as usual, if fp isn't NULL, use it,
-  // otherwise, do nothing.
-  StackFrameIterator(Isolate* isolate, bool use_top, Address fp, Address sp);
-
   StackFrame* frame() const {
     ASSERT(!done());
     return frame_;
@@ -798,6 +797,12 @@ class StackFrameIterator BASE_EMBEDDED {
   void Advance() { (this->*advance_)(); }
 
  private:
+  // An iterator that can start from a given FP address.
+  // If use_top, then work as usual, if fp isn't NULL, use it,
+  // otherwise, do nothing. This constructor is used to create
+  // StackFrameIterator for "safe" stack iteration.
+  StackFrameIterator(Isolate* isolate, bool use_top, Address fp, Address sp);
+
   // Go back to the first frame.
   void Reset();
 
@@ -811,6 +816,7 @@ class StackFrameIterator BASE_EMBEDDED {
   Address fp_;
   Address sp_;
   void (StackFrameIterator::*advance_)();
+  const bool can_access_heap_objects_;
 
   StackHandler* handler() const {
     ASSERT(!done());
@@ -879,8 +885,6 @@ class SafeStackFrameIterator BASE_EMBEDDED {
   bool done() const { return iteration_done_ || iterator_.done(); }
   void Advance();
 
-  static bool is_active(Isolate* isolate);
-
  private:
   void AdvanceOneFrame();
 
@@ -921,20 +925,6 @@ class SafeStackFrameIterator BASE_EMBEDDED {
   static bool IsValidTop(Isolate* isolate,
                          Address low_bound, Address high_bound);
 
-  // This is a nasty hack to make sure the active count is incremented
-  // before the constructor for the embedded iterator is invoked. This
-  // is needed because the constructor will start looking at frames
-  // right away and we need to make sure it doesn't start inspecting
-  // heap objects.
-  class ActiveCountMaintainer BASE_EMBEDDED {
-   public:
-    explicit ActiveCountMaintainer(Isolate* isolate);
-    ~ActiveCountMaintainer();
-   private:
-    Isolate* isolate_;
-  };
-
-  ActiveCountMaintainer maintainer_;
   StackAddressValidator stack_validator_;
   const bool is_valid_top_;
   const bool is_valid_fp_;
diff --git a/src/isolate.h b/src/isolate.h
index 0552ef7..b1e2b07 100644
--- a/src/isolate.h
+++ b/src/isolate.h
@@ -368,8 +368,6 @@ typedef List<HeapObject*, PreallocatedStorageAllocationPolicy> DebugObjectCache;
   /* AstNode state. */                                                         \
   V(int, ast_node_id, 0)                                                       \
   V(unsigned, ast_node_count, 0)                                               \
-  /* SafeStackFrameIterator activations count. */                              \
-  V(int, safe_stack_iterator_counter, 0)                                       \
   V(bool, observer_delivery_pending, false)                                    \
   V(HStatistics*, hstatistics, NULL)                                           \
   V(HTracer*, htracer, NULL)                                                   \