Use symbols when looking up fields in a class

runtime/vm/object.cc

Index: runtime/vm/object.cc
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index 44e9d23312d593d64b356ef300bad1de9e43d571..6f514a361b5679d9ca3b5b411f35536300c0190b 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -4161,26 +4161,80 @@ RawField* Class::LookupField(const String& name, MemberKind kind) const {
   ASSERT(!flds.IsNull());
   intptr_t len = flds.Length();
   Field& field = thread->FieldHandle();
+  if (name.IsSymbol()) {
+    // Use fast raw pointer string compare for symbols.
+    for (intptr_t i = 0; i < len; i++) {
+      field ^= flds.At(i);
+      if (name.raw() == field.name()) {
+        if (kind == kInstance) {
+          return field.is_static() ? Field::null() : field.raw();
+        } else if (kind == kStatic) {
+          return field.is_static() ? field.raw() : Field::null();
+        }
+        ASSERT(kind == kAny);
+        return field.raw();
+      }
+    }
+  } else {
+    String& field_name = thread->StringHandle();
+    for (intptr_t i = 0; i < len; i++) {
+      field ^= flds.At(i);
+      field_name ^= field.name();
+      if (name.Equals(field_name)) {
+        if (kind == kInstance) {
+          return field.is_static() ? Field::null() : field.raw();
+        } else if (kind == kStatic) {
+          return field.is_static() ? field.raw() : Field::null();
+        }
+        ASSERT(kind == kAny);
+        return field.raw();

[siva, 2016/04/07 22:51:30]

Not sure if it is worth extracting the code inside the if block into :

static RawField* getField(MemberKind kind, const Field& field) { ... }

and do return getField(kind, field); in the two places.

[hausner, 2016/04/07 23:37:03]

I hesitate to create a mini function to factor out 5 lines of code that
otherwise don't have any use. In the *AllowPrivate version of this function, I
pulled this check out into the caller. That requires the caller to allocate
another Handle to do the is_static() check. I didn't want that for the fast
path.

If you insist, I'll do it in a follow-up CL.

+      }
+    }
+  }
+  return Field::null();
+}
+
+
+RawField* Class::LookupFieldAllowPrivate(const String& name) const {
+  // Use slow string compare, ignoring privacy name mangling.
+  Thread* thread = Thread::Current();
+  if (EnsureIsFinalized(thread) != Error::null()) {
+    return Field::null();
+  }
+  REUSABLE_ARRAY_HANDLESCOPE(thread);
+  REUSABLE_FIELD_HANDLESCOPE(thread);
+  REUSABLE_STRING_HANDLESCOPE(thread);
+  Array& flds = thread->ArrayHandle();
+  flds ^= fields();
+  ASSERT(!flds.IsNull());
+  intptr_t len = flds.Length();
+  Field& field = thread->FieldHandle();
   String& field_name = thread->StringHandle();
   for (intptr_t i = 0; i < len; i++) {
     field ^= flds.At(i);
     field_name ^= field.name();
     if (String::EqualsIgnoringPrivateKey(field_name, name)) {
-      if (kind == kInstance) {
-        if (!field.is_static()) {
-          return field.raw();
-        }
-      } else if (kind == kStatic) {
-        if (field.is_static()) {
-          return field.raw();
-        }
-      } else if (kind == kAny) {
-        return field.raw();
-      }
-      return Field::null();
+      return field.raw();
     }
   }
-  // No field found.
+  return Field::null();
+}
+
+
+RawField* Class::LookupInstanceFieldAllowPrivate(const String& name) const {
+  Field& field = Field::Handle(LookupFieldAllowPrivate(name));
+  if (!field.IsNull() && !field.is_static()) {
+    return field.raw();
+  }
+  return Field::null();
+}
+
+
+RawField* Class::LookupStaticFieldAllowPrivate(const String& name) const {
+  Field& field = Field::Handle(LookupFieldAllowPrivate(name));
+  if (!field.IsNull() && field.is_static()) {
+    return field.raw();
+  }
   return Field::null();
 }
 
@@ -9414,14 +9468,13 @@ RawObject* Library::GetMetadata(const Object& obj) const {
 
 static bool ShouldBePrivate(const String& name) {
   return
-      (name.Length() >= 1 &&
-       name.CharAt(0) == '_') ||
+      (name.Length() >= 1 && name.CharAt(0) == '_') ||
       (name.Length() >= 5 &&
-       (name.CharAt(4) == '_' &&
-        (name.CharAt(0) == 'g' || name.CharAt(0) == 's') &&
-        name.CharAt(1) == 'e' &&
-        name.CharAt(2) == 't' &&
-        name.CharAt(3) == ':'));
+        (name.CharAt(4) == '_' &&
+          (name.CharAt(0) == 'g' || name.CharAt(0) == 's') &&
+          name.CharAt(1) == 'e' &&
+          name.CharAt(2) == 't' &&
+          name.CharAt(3) == ':'));

[sra1, 2016/04/08 20:30:06]

Testing ':' before 'g' / 's' will fail faster.

[hausner, 2016/04/08 20:41:20]

The test (name.CharAt(4) == '_') eliminates practically all non-getter and
non-setter names. I would not lose sleep over this :)

 }
 
 
@@ -9436,6 +9489,7 @@ RawObject* Library::ResolveName(const String& name) const {
     // are not cached. This reduces the size of the the cache.
     return obj.raw();
   }
+
   String& accessor_name = String::Handle(Field::LookupGetterSymbol(name));
   if (!accessor_name.IsNull()) {
     obj = LookupLocalObject(accessor_name);
@@ -9449,6 +9503,7 @@ RawObject* Library::ResolveName(const String& name) const {
       obj = LookupImportedObject(name);
     }
   }
+
   AddToResolvedNamesCache(name, obj);
   return obj.raw();
 }