Refactor the way we collect the information for associating type-related infos

src/type-info.cc

 // Copyright 2011 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 43 matching lines @@
   } else {
     info = TypeInfo::Unknown();
   }
   return info;
 }
 
 
 TypeFeedbackOracle::TypeFeedbackOracle(Handle<Code> code,
                                        Handle<Context> global_context) {
   global_context_ = global_context;
-  PopulateMap(code);
+  BuildDictionary(code);
   ASSERT(reinterpret_cast<Address>(*dictionary_.location()) != kHandleZapValue);
 }
 
 
 Handle<Object> TypeFeedbackOracle::GetInfo(unsigned ast_id) {
   int entry = dictionary_->FindEntry(ast_id);
   return entry != NumberDictionary::kNotFound
       ? Handle<Object>(dictionary_->ValueAt(entry))
       : Isolate::Current()->factory()->undefined_value();
 }
@@ skipping 308 matching lines @@
     ZoneMapList* types = new ZoneMapList(4);
     ASSERT(object->IsCode());
     isolate->stub_cache()->CollectMatchingMaps(types, *name, flags);
     return types->length() > 0 ? types : NULL;
   } else {
     return NULL;
   }
 }
 
 
+// Things are a bit tricky here: The iterator for the RelocInfos and the infos
+// themselves are not GC-safe, so we first get all infos, then we create the
+// dictionary (possibly triggering GC), and finally we relocate the collected
+// infos before we process them.
+void TypeFeedbackOracle::BuildDictionary(Handle<Code> code) {
+  AssertNoAllocation no_allocation;
+  HandleScope scope;
+  List<RelocInfo> infos(16);
+  GetRelocInfos(code, &infos);
+  CreateDictionary(code, &infos);
+  ProcessRelocInfos(&infos);
+  // Allocate handle in the parent scope.
+  dictionary_ = scope.CloseAndEscape(dictionary_);
+}
+
+
+void TypeFeedbackOracle::GetRelocInfos(Handle<Code> code,
+                                       List<RelocInfo>* infos) {
+  int mask = RelocInfo::ModeMask(RelocInfo::CODE_TARGET_WITH_ID);
+  for (RelocIterator it(*code, mask); !it.done(); it.next()) {
+    infos->Add(*it.rinfo());
+  }
+}
+
+
+void TypeFeedbackOracle::CreateDictionary(Handle<Code> code,
+                                          List<RelocInfo>* infos) {
+  DisableAssertNoAllocation allocation_allowed;
+  byte* old_start = (*code)->instruction_start();

[Mads Ager (chromium), 2011/06/20 08:57:38]

You shouldn't need the explicit dereference here?

code->instruction_start()

[Sven Panne, 2011/06/20 10:16:20]

Done.

+  dictionary_ = FACTORY->NewNumberDictionary(infos->length());
+  byte* new_start = (*code)->instruction_start();

[Mads Ager (chromium), 2011/06/20 08:57:38]

Ditto.

[Sven Panne, 2011/06/20 10:16:20]

Done.

+  RelocateRelocInfos(infos, old_start, new_start);
+}
+
+
+void TypeFeedbackOracle::RelocateRelocInfos(List<RelocInfo>* infos,
+                                            byte* old_start,
+                                            byte* new_start) {
+  for (int i = 0; i < infos->length(); i++) {
+    RelocInfo* info = &(*infos)[i];
+    info->set_pc(new_start + (info->pc() - old_start));
+  }
+}
+
+
+void TypeFeedbackOracle::ProcessRelocInfos(List<RelocInfo>* infos) {
+  for (int i = 0; i < infos->length(); i++) {
+    unsigned ast_id = static_cast<unsigned>((*infos)[i].data());
+    Code* target = Code::GetCodeFromTargetAddress((*infos)[i].target_address());
+    ProcessTarget(ast_id, target);
+  }
+}
+
+
+void TypeFeedbackOracle::ProcessTarget(unsigned ast_id, Code* target) {
+  switch (target->kind()) {
+    case Code::LOAD_IC:
+    case Code::STORE_IC:
+    case Code::CALL_IC:
+    case Code::KEYED_CALL_IC:
+      if (target->ic_state() == MONOMORPHIC) {
+        if (target->kind() == Code::CALL_IC &&
+            target->check_type() != RECEIVER_MAP_CHECK) {
+          SetInfo(ast_id,  Smi::FromInt(target->check_type()));
+        } else {
+          Object* map = target->FindFirstMap();
+          SetInfo(ast_id, map == NULL ? static_cast<Object*>(target) : map);
+        }
+      } else if (target->ic_state() == MEGAMORPHIC) {
+        SetInfo(ast_id, target);
+      }
+      break;
+
+    case Code::KEYED_LOAD_IC:
+    case Code::KEYED_STORE_IC:
+      if (target->ic_state() == MONOMORPHIC ||
+          target->ic_state() == MEGAMORPHIC) {
+        SetInfo(ast_id, target);
+      }
+      break;
+
+    case Code::UNARY_OP_IC:
+    case Code::BINARY_OP_IC:
+    case Code::COMPARE_IC:
+      SetInfo(ast_id, target);

[Mads Ager (chromium), 2011/06/20 08:57:38]

Didn't we have some filtering in the old code for generic states here (for
compare and binary op ics)? Have you removed that filtering on purpose to keep
this code simpler and because they are just ignored later?

[Sven Panne, 2011/06/20 10:16:20]

We *had* some filtering code here, but I removed it very deliberately. The
result of the corresponding oracle calls later are the same with or without them
(as is the performance), and "no code is good code". Let's not 'optimize' too
early, let's keep the initialization code as simple as possible instead. It
would be even better if ProcessTarget simply did a SetInfo(ast_id, target) for
*all* cases and all real logic was in the various oracle calls, but it's not
obvious if this would hurt performance or not, so I did this (intermediate) step
first.

+      break;
+
+    default:
+      break;
+  }
+}
+
+
 void TypeFeedbackOracle::SetInfo(unsigned ast_id, Object* target) {
   ASSERT(dictionary_->FindEntry(ast_id) == NumberDictionary::kNotFound);
   MaybeObject* maybe_result = dictionary_->AtNumberPut(ast_id, target);
   USE(maybe_result);
 #ifdef DEBUG
   Object* result = NULL;
   // Dictionary has been allocated with sufficient size for all elements.
   ASSERT(maybe_result->ToObject(&result));
   ASSERT(*dictionary_ == result);
 #endif
 }
 
-
-void TypeFeedbackOracle::PopulateMap(Handle<Code> code) {
-  Isolate* isolate = Isolate::Current();
-  HandleScope scope(isolate);
-
-  const int kInitialCapacity = 16;
-  List<int> code_positions(kInitialCapacity);
-  List<unsigned> ast_ids(kInitialCapacity);
-  CollectIds(*code, &code_positions, &ast_ids);
-
-  ASSERT(dictionary_.is_null());  // Only initialize once.
-  dictionary_ = isolate->factory()->NewNumberDictionary(
-      code_positions.length());
-
-  const int length = code_positions.length();
-  ASSERT(ast_ids.length() == length);
-  for (int i = 0; i < length; i++) {
-    AssertNoAllocation no_allocation;
-    RelocInfo info(code->instruction_start() + code_positions[i],
-                   RelocInfo::CODE_TARGET, 0);
-    Code* target = Code::GetCodeFromTargetAddress(info.target_address());
-    unsigned id = ast_ids[i];
-    InlineCacheState state = target->ic_state();
-    Code::Kind kind = target->kind();
-
-    if (kind == Code::BINARY_OP_IC ||
-        kind == Code::UNARY_OP_IC ||
-        kind == Code::COMPARE_IC) {
-      SetInfo(id, target);
-    } else if (state == MONOMORPHIC) {
-      if (kind == Code::KEYED_LOAD_IC ||
-          kind == Code::KEYED_STORE_IC) {
-        SetInfo(id, target);
-      } else if (kind != Code::CALL_IC ||
-                 target->check_type() == RECEIVER_MAP_CHECK) {
-        Map* map = target->FindFirstMap();
-        if (map == NULL) {
-          SetInfo(id, target);
-        } else {
-          SetInfo(id, map);
-        }
-      } else {
-        ASSERT(target->kind() == Code::CALL_IC);
-        CheckType check = target->check_type();
-        ASSERT(check != RECEIVER_MAP_CHECK);
-        SetInfo(id,  Smi::FromInt(check));
-      }
-    } else if (state == MEGAMORPHIC) {
-      SetInfo(id, target);
-    }
-  }
-  // Allocate handle in the parent scope.
-  dictionary_ = scope.CloseAndEscape(dictionary_);
-}
-
-
-void TypeFeedbackOracle::CollectIds(Code* code,
-                                    List<int>* code_positions,
-                                    List<unsigned>* ast_ids) {
-  AssertNoAllocation no_allocation;
-  int mask = RelocInfo::ModeMask(RelocInfo::CODE_TARGET_WITH_ID);
-  for (RelocIterator it(code, mask); !it.done(); it.next()) {
-    RelocInfo* info = it.rinfo();
-    ASSERT(RelocInfo::IsCodeTarget(info->rmode()));
-    Code* target = Code::GetCodeFromTargetAddress(info->target_address());
-    if (target->is_inline_cache_stub()) {
-      InlineCacheState state = target->ic_state();
-      Code::Kind kind = target->kind();
-      if (kind == Code::BINARY_OP_IC) {
-        if (target->binary_op_type() ==
-            BinaryOpIC::GENERIC) {
-          continue;
-        }
-      } else if (kind == Code::COMPARE_IC) {
-        if (target->compare_state() == CompareIC::GENERIC) continue;
-      } else {
-        if (state != MONOMORPHIC && state != MEGAMORPHIC) continue;
-      }
-      code_positions->Add(
-          static_cast<int>(info->pc() - code->instruction_start()));
-      ASSERT(ast_ids->length() == 0 ||
-             (*ast_ids)[ast_ids->length()-1] !=
-             static_cast<unsigned>(info->data()));
-      ast_ids->Add(static_cast<unsigned>(info->data()));
-    }
-  }
-}
-
 } }  // namespace v8::internal