Encapsulate element handling into a class keyed on ElementsKind

src/elements.cc

Index: src/elements.cc
diff --git a/src/elements.cc b/src/elements.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bac6e761c7dbf7f84856f57c2f14f9407d1857a5
--- /dev/null
+++ b/src/elements.cc
@@ -0,0 +1,330 @@
+// 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
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "v8.h"
+
+#include "objects.h"
+#include "elements.h"
+
+namespace v8 {
+namespace internal {
+
+
+// Base class for element handler implementations. Contains the
+// the common logic for objects with different ElementsKinds.
+// Subclasses must specialize method for which the element
+// implementation differs from the base class implementation.
+//
+// This class is intended to be used in the following way:
+//
+//   class SomeElementsHandlerImpl :

[Lasse Reichstein, 2011/08/01 09:35:54]

I really, really dislike adding "Impl" to a class name. Any non-purely-virtual
class is an implementation, so "Impl" is vacuous.
Personally, I use "ElementsHandlerBase" for the base class, and just
"SomeElementsHandler" for the subclass, but that's not really optimal either.

[danno, 2011/08/02 13:37:46]

Done.

+//       public ElementsHandlerImpl<SomeElementsHandlerImpl,
+//                                  BackingStoreClass> {
+//     ...
+//   }
+//
+// This is an example of Curiously recurring template pattern

[Lasse Reichstein, 2011/08/01 09:35:54]

Either don't capitalize "Curiously" (and prefix it with a "the"), or
capitalize/title-case all four words.

[danno, 2011/08/02 13:37:46]

Done.

+// (see http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern).
+// We use CRTP to guarantee aggressive compile time optimizations (i.e.
+// inlining and specialization of SomeElementsHandlerImpl methods).
+template <typename ConcreteHandlerImpl, typename BackingStoreClass>
+class ElementsHandlerImpl {
+ public:
+  inline static bool GetWithReceiver(JSObject* obj,
+                                     Object* receiver,
+                                     uint32_t index,
+                                     MaybeObject** result) {

[Lasse Reichstein, 2011/08/01 09:35:54]

Why MaybeObject as result, if you always return an Object?

[danno, 2011/08/02 13:37:46]

The FixedDoubleArray handler (and some of the external array handlers) allocate
heap numbers and can cause a GC, so MaybeObject is correct.
 
On 2011/08/01 09:35:54, Lasse Reichstein wrote:

+    if (index < ConcreteHandlerImpl::GetLength(obj)) {
+      BackingStoreClass* backing_store =
+          ConcreteHandlerImpl::GetBackingStore(obj);
+      MaybeObject* value = backing_store->get(index);
+      if (!value->IsTheHole()) {
+        *result = value;
+        return true;
+      };
+    }
+    return false;
+  }
+
+ protected:
+  inline static BackingStoreClass* GetBackingStore(JSObject* obj) {
+    return BackingStoreClass::cast(obj->elements());
+  }
+
+  inline static uint32_t GetLength(JSObject* obj) {
+    return ConcreteHandlerImpl::GetBackingStore(obj)->length();
+  }
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(ElementsHandlerImpl);
+};
+
+
+class FastElementsHandlerImpl
+    : public ElementsHandlerImpl<FastElementsHandlerImpl, FixedArray> {
+};
+
+
+class FastDoubleElementsHandlerImpl
+    : public ElementsHandlerImpl<FastDoubleElementsHandlerImpl,
+                                 FixedDoubleArray> {
+};
+
+
+// Super class for all external element arrays, whose handler implementation is
+// identical except for specialization.

[Lasse Reichstein, 2011/08/01 09:35:54]

You specialize the only real function, so it's pretty non-identical.

[danno, 2011/08/02 13:37:46]

Done.

+template<typename ConcreteExternalElementHandlerImpl,
+         typename ExternalArray>
+class ExternalElementsHandlerImpl
+    : public ElementsHandlerImpl<ConcreteExternalElementHandlerImpl,
+                                 ExternalArray> {
+ public:
+  inline static bool GetWithReceiver(JSObject* obj,
+                                     Object* receiver,
+                                     uint32_t index,
+                                     MaybeObject** result) {
+    if (index < ConcreteExternalElementHandlerImpl::GetLength(obj)) {
+      ExternalArray* backing_store =
+          ConcreteExternalElementHandlerImpl::GetBackingStore(obj);
+      *result = backing_store->get(index);
+    } else {
+      *result = obj->GetHeap()->undefined_value();
+    }
+    return true;
+  }
+};
+
+
+class ExternalByteElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<ExternalByteElementsHandlerImpl,
+                                         ExternalByteArray> {
+};
+
+
+class ExternalUnsignedByteElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<
+  ExternalUnsignedByteElementsHandlerImpl, ExternalUnsignedByteArray> {
+};
+
+
+class ExternalShortElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<ExternalShortElementsHandlerImpl,
+                                         ExternalShortArray> {
+};
+
+
+class ExternalUnsignedShortElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<
+  ExternalUnsignedShortElementsHandlerImpl, ExternalUnsignedShortArray> {
+};
+
+
+class ExternalIntElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<ExternalIntElementsHandlerImpl,
+                                         ExternalIntArray> {
+};
+
+
+class ExternalUnsignedIntElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<ExternalUnsignedIntElementsHandlerImpl,
+                                         ExternalUnsignedIntArray> {
+};
+
+
+class ExternalFloatElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<ExternalFloatElementsHandlerImpl,
+                                         ExternalFloatArray> {
+};
+
+
+class ExternalDoubleElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<ExternalDoubleElementsHandlerImpl,
+                                         ExternalDoubleArray> {
+};
+
+
+class PixelElementsHandlerImpl
+    : public ExternalElementsHandlerImpl<PixelElementsHandlerImpl,
+                                         ExternalPixelArray> {
+};
+
+
+class DictionaryElementsHandlerImpl
+    : public ElementsHandlerImpl<DictionaryElementsHandlerImpl,
+                                 NumberDictionary> {
+ public:
+  inline static bool GetNumberDictionaryElement(

[Rico, 2011/08/01 07:33:27]

Do we use this function outside this class, otherwise just make it private?

[danno, 2011/08/02 13:37:46]

Yes, the NonStrictArgument accessor uses this.
On 2011/08/01 07:33:27, Rico wrote:

+      JSObject* obj,
+      Object* receiver,
+      NumberDictionary* backing_store,
+      uint32_t index,
+      MaybeObject** result) {
+    int entry = backing_store->FindEntry(index);
+    if (entry != NumberDictionary::kNotFound) {
+      Object* element = backing_store->ValueAt(entry);
+      PropertyDetails details = backing_store->DetailsAt(entry);
+      if (details.type() == CALLBACKS) {
+        *result = obj->GetElementWithCallback(receiver,
+                                              element,
+                                              index,
+                                              obj);
+      } else {
+        *result = element;
+      }
+      return true;
+    }
+    return false;
+  }
+
+  inline static bool GetWithReceiver(JSObject* obj,
+                                     Object* receiver,
+                                     uint32_t index,
+                                     MaybeObject** result) {
+    NumberDictionary* backing_store = GetBackingStore(obj);
+    return GetNumberDictionaryElement(obj,
+                                      receiver,
+                                      backing_store,
+                                      index,
+                                      result);
+  }
+
+ protected:
+  inline static NumberDictionary* GetBackingStore(JSObject* obj) {
+    return NumberDictionary::cast(obj->element_dictionary());

[Lasse Reichstein, 2011/08/01 09:35:54]

Why specialize this? 
The element_dictionary method does exactly the same as the generic
GetBackingStore function, and the extra cast to NumberDictionary here isn't
needed, since element_dictionary returns a NumberDictionary already.

[danno, 2011/08/02 13:37:46]

Done.

+  }
+};
+
+
+class NonStrictArgumentsElementsHandlerImpl
+    : public ElementsHandlerImpl<NonStrictArgumentsElementsHandlerImpl,
+                                 FixedArray> {
+ public:
+  inline static bool GetWithReceiver(JSObject* obj,
+                                     Object* receiver,
+                                     uint32_t index,
+                                     MaybeObject** result) {
+    FixedArray* parameter_map = GetBackingStore(obj);
+    uint32_t length = parameter_map->length();
+    Object* probe =
+        (index < length - 2) ? parameter_map->get(index + 2) : NULL;
+    if (probe != NULL && !probe->IsTheHole()) {
+      Context* context = Context::cast(parameter_map->get(0));
+      int context_index = Smi::cast(probe)->value();
+      *result = context->get(context_index);
+      ASSERT(!context->get(context_index)->IsTheHole());
+      return true;
+    } else {
+      // Object is not mapped, defer to the arguments.
+      FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
+      if (arguments->IsDictionary()) {
+        return DictionaryElementsHandlerImpl::GetNumberDictionaryElement(
+            obj,
+            receiver,
+            NumberDictionary::cast(arguments),
+            index,
+            result);
+      } else if (index < static_cast<uint32_t>(arguments->length())) {
+        Object* value = arguments->get(index);
+        if (!value->IsTheHole()) {
+          *result = value;
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+};
+
+
+template<typename Impl>
+class ConcreteElementsHandler : public ElementsHandler {

[Lasse Reichstein, 2011/08/01 09:35:54]

"Concrete" sounds like a weasel-word too. And it's not a word I would use for a
template :)
If ElementsHandler is abstract, call it AbstractElementsHandler (except that
it's bad naming, because it makes no sense for an is-a relationship, so call it
something else). 
Being concrete should be the default.

I.e., a class structure something like:

 class ElementsGetter {
  public:
   virtual bool GetWithReceiver(JSObject* obj, ...) = 0;
   static ElementsGetter* ForKind(JSObject::ElementsKind elements_kind);
  private:
   ElementsGetter* getters[];
 }

 template <typename GetterType, typename BackingStoreType>
 class TemplatedElementsGetter: public ElementsGetter {
  public:
   virtual bool GetWithReceiver(...) { ... }
  private: 
   BackingStoreType* GetBackingStore(JSObject* obj) { ... }
   ...
 }

 class PixelElementsGetter : public TemplatedElementsGetter<PixelElementsGetter,
ExternalPixelArray> { };


As a different idea, maybe just make it an array of functions that you can call
directly, instead of an array of objects that you call virtual functions on.
Should save an indirection.
(Yes, I'm a functional programmer by heart :)

[danno, 2011/08/02 13:37:46]

Fixed. I prefer the classes, because they later will be the home for not just
the GetWithReceiver, but other accessors, too.

+ public:
+  ConcreteElementsHandler() {}
+
+  virtual bool GetWithReceiver(JSObject* obj,
+                               Object* receiver,
+                               uint32_t index,
+                               MaybeObject** result) {
+    return Impl::GetWithReceiver(obj,
+                                 receiver,
+                                 index,
+                                 result);

[Lasse Reichstein, 2011/08/01 09:35:54]

Why is this split into ElementsHandler and ElementsHandlerImpl at all?
Can't you just have your handler struct contain the ElementsHandlerImpl
subclasses directly, and let those GetWithReceiver functions be virtual.
I.e. remove the classes you call ElementsHandler and ConcreteElementsHandler
completely?

[danno, 2011/08/02 13:37:46]

Done.

+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ConcreteElementsHandler);
+};
+
+
+struct ConcreteElementHandlers {
+  ConcreteElementsHandler<FastElementsHandlerImpl>
+      fast_elements_handler;
+  ConcreteElementsHandler<FastDoubleElementsHandlerImpl>
+      fast_double_elements_handler;
+  ConcreteElementsHandler<DictionaryElementsHandlerImpl>
+      dictionary_elements_handler;
+  ConcreteElementsHandler<NonStrictArgumentsElementsHandlerImpl>
+      non_strict_arguments_elements_handler;
+  ConcreteElementsHandler<ExternalByteElementsHandlerImpl>
+      byte_elements_handler;
+  ConcreteElementsHandler<ExternalUnsignedByteElementsHandlerImpl>
+      unsigned_byte_elements_handler;
+  ConcreteElementsHandler<ExternalShortElementsHandlerImpl>
+      short_elements_handler;
+  ConcreteElementsHandler<ExternalUnsignedShortElementsHandlerImpl>
+      unsigned_short_elements_handler;
+  ConcreteElementsHandler<ExternalIntElementsHandlerImpl>
+      int_elements_handler;
+  ConcreteElementsHandler<ExternalUnsignedIntElementsHandlerImpl>
+      unsigned_int_elements_handler;
+  ConcreteElementsHandler<ExternalFloatElementsHandlerImpl>
+      float_elements_handler;
+  ConcreteElementsHandler<ExternalDoubleElementsHandlerImpl>
+      double_elements_handler;
+  ConcreteElementsHandler<PixelElementsHandlerImpl>
+      pixel_elements_handler;
+} element_handlers;

[Lasse Reichstein, 2011/08/01 09:35:54]

Seems like a big static initializer. Might upset the Chrome people.

[danno, 2011/08/02 13:37:46]

Done.

+
+
+ElementsHandler* ElementsHandler::elements_handler_table_[] = {
+  &element_handlers.fast_elements_handler,
+  &element_handlers.fast_double_elements_handler,
+  &element_handlers.dictionary_elements_handler,
+  &element_handlers.non_strict_arguments_elements_handler,
+  &element_handlers.byte_elements_handler,
+  &element_handlers.unsigned_byte_elements_handler,
+  &element_handlers.short_elements_handler,
+  &element_handlers.unsigned_short_elements_handler,
+  &element_handlers.int_elements_handler,
+  &element_handlers.unsigned_int_elements_handler,
+  &element_handlers.float_elements_handler,
+  &element_handlers.double_elements_handler,
+  &element_handlers.pixel_elements_handler
+};
+
+
+} }  // namespace v8::internal