Encapsulate element handling into a class keyed on ElementsKind

Encapsulate element handling into a class keyed on ElementsKind 

Advantage is that it's much easier to add new element types (like FAST_SMI_ELEMENTS), and that handling logic for each element kind is (more) consolidated. 

Currently, only GetElementsWithReceiver uses the new encapsulation, but the goal is to move much more element functionality into the class incrementally. 

BUG=none
TEST=none

Committed: http://code.google.com/p/v8/source/detail?r=8810

[danno, 2011-07-29 11:10:40]

Please take a look.

[danno, 2011-07-29 12:21:12]

adding kevin

[Jakob Kummerow, 2011-07-30 10:29:21]

Drive-by nits :-)

For the high-level evaluation of this approach, I defer to others with stronger
opinions. FWIW, I like it. A little complicated at first sight, maybe, but
overall cleaner and more maintainable than what it replaces.

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc#newcode45
src/elements.cc:45: //       public ElementsHandlerImpl<SomeElementsHandlerImpl>
{
nit: second template argument (BackingStoreClass) is missing in this example.

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc#newcode87
src/elements.cc:87: : public ElementsHandlerImpl< FastElementsHandlerImpl,
FixedArray> {
nit: extra space

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc#newcode250
src/elements.cc:250: struct ConcreteElementHandlers {
nit: for consistency, change order to match elements_handler_table_? (I.e. move
up "dictionary" and "non_strict_arguments".)

http://codereview.chromium.org/7527001/diff/12001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7527001/diff/12001/src/objects.cc#newcode8949
src/objects.cc:8949: #if 0
nit: If we don't need this code anymore, remove it.

http://codereview.chromium.org/7527001/diff/12001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7527001/diff/12001/src/objects.h#newcode2024
src/objects.h:2024: //  MaybeObject* GetElementPostInterceptor(Object* receiver,
uint32_t index);
nit: remove the line instead of commenting it out.

http://codereview.chromium.org/7527001/diff/12001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

http://codereview.chromium.org/7527001/diff/12001/tools/gyp/v8.gyp#newcode491
tools/gyp/v8.gyp:491: '../../src/elements.cc',
Should add elements.h here as well.

[danno, 2011-07-30 13:10:10]

feedback address.

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc#newcode45
src/elements.cc:45: //       public ElementsHandlerImpl<SomeElementsHandlerImpl>
{
On 2011/07/30 10:29:21, jkummerow wrote:
> nit: second template argument (BackingStoreClass) is missing in this example.

Done.

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc#newcode87
src/elements.cc:87: : public ElementsHandlerImpl< FastElementsHandlerImpl,
FixedArray> {
On 2011/07/30 10:29:21, jkummerow wrote:
> nit: extra space

Done.

http://codereview.chromium.org/7527001/diff/12001/src/elements.cc#newcode250
src/elements.cc:250: struct ConcreteElementHandlers {
On 2011/07/30 10:29:21, jkummerow wrote:
> nit: for consistency, change order to match elements_handler_table_? (I.e.
move
> up "dictionary" and "non_strict_arguments".)

Done.

http://codereview.chromium.org/7527001/diff/12001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7527001/diff/12001/src/objects.cc#newcode8949
src/objects.cc:8949: #if 0
On 2011/07/30 10:29:21, jkummerow wrote:
> nit: If we don't need this code anymore, remove it.

Done.

http://codereview.chromium.org/7527001/diff/12001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7527001/diff/12001/src/objects.h#newcode2024
src/objects.h:2024: //  MaybeObject* GetElementPostInterceptor(Object* receiver,
uint32_t index);
On 2011/07/30 10:29:21, jkummerow wrote:
> nit: remove the line instead of commenting it out.

Done.

http://codereview.chromium.org/7527001/diff/12001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

http://codereview.chromium.org/7527001/diff/12001/tools/gyp/v8.gyp#newcode491
tools/gyp/v8.gyp:491: '../../src/elements.cc',
On 2011/07/30 10:29:21, jkummerow wrote:
> Should add elements.h here as well.

Done.

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

LGTM, I like the approach, although it is quite different from what we do
elsewhere in the V8 code-base. Adding Lasse as reviewer as well, he has a small
thing for templates :-)

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode180
src/elements.cc:180: inline static bool GetNumberDictionaryElement(
Do we use this function outside this class, otherwise just make it private?

http://codereview.chromium.org/7527001/diff/15002/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode36
src/elements.h:36: // Abstract base class for handles that can operate on object
with differing
on object -> an object or objects

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

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode44
src/elements.cc:44: //   class SomeElementsHandlerImpl :
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.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode50
src/elements.cc:50: // This is an example of Curiously recurring template
pattern
Either don't capitalize "Curiously" (and prefix it with a "the"), or
capitalize/title-case all four words.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode60
src/elements.cc:60: MaybeObject** result) {
Why MaybeObject as result, if you always return an Object?

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode99
src/elements.cc:99: // identical except for specialization.
You specialize the only real function, so it's pretty non-identical.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode217
src/elements.cc:217: return NumberDictionary::cast(obj->element_dictionary());
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.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode264
src/elements.cc:264: class ConcreteElementsHandler : public ElementsHandler {
"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 :)

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode275
src/elements.cc:275: result);
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?

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode310
src/elements.cc:310: } element_handlers;
Seems like a big static initializer. Might upset the Chrome people.

http://codereview.chromium.org/7527001/diff/15002/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode38
src/elements.h:38: class ElementsHandler {
What does "Handler" mean. Seems too generic to be meaningful.

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h#newcode1604
src/objects-inl.h:1604: return get(index)->IsTheHole();
Is this really used so much that it needs this shorthand?
I would have thought that in the cases where you need the check, you also need
the value afterwards, if it isn't the hole.

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h#newcode1680
src/objects-inl.h:1680: return is_the_hole_nan(READ_DOUBLE_FIELD(this, offset));
Probably stupid question, but can this read be unaligned?

http://codereview.chromium.org/7527001/diff/15002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/objects.h#newcode2070
src/objects.h:2070: class FixedArrayBase: public HeapObject {
Preferably this should be called FixedArray, as the generic superclass of all
fixed-size arrays, and the original FixedArray should be ObjectFixedArray.
I can see why it wasn't done, though, having done the same thing myself
elsewhere :)

[Kevin Millikin (Chromium), 2011-08-01 10:18:23]

My high level comment is the same as Lasse's: I think there is one too many
levels of indirection.  It should be possible to roll the ...Impl class into the
ElementHandlers themselves.

You could also make the handler singletons static members of ElementHandler
instead of in a struct on the side (somewhat like we have StackFrame singletons
in the StackFrameIterator).

http://codereview.chromium.org/7527001/diff/15002/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode48
src/elements.h:48: inline static ElementsHandler* ForKind(JSObject::ElementsKind
elements_kind) {
No need for 'inline'.  Maybe the function name/signature should be something
like SingletonFor(JSObject::ElementsKind).

[Lasse Reichstein, 2011-08-01 10:47:13]

http://codereview.chromium.org/7527001/diff/15002/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode48
src/elements.h:48: inline static ElementsHandler* ForKind(JSObject::ElementsKind
elements_kind) {
We should probably not call it anything with "Singleton", since that's a less
general (anti-)pattern (and we don't implement each class as a Singleton class -
which is a good thing!).
This is more like an object-based enum (q.v. the Java enums, where,
incidentally, "enum Foo" is implemented internally as "class Foo extends
Enum<Foo>" :)

[Lasse Reichstein, 2011-08-02 12:23:46]

http://codereview.chromium.org/7527001/diff/15002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/objects.h#newcode74
src/objects.h:74: //       - FixedArray
I suddently remembered that this documentation is out-of-date now that there is
a FixedArrayBase. If possible, make it match reality :)

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

please take another look

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode44
src/elements.cc:44: //   class SomeElementsHandlerImpl :
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> 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.

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode50
src/elements.cc:50: // This is an example of Curiously recurring template
pattern
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> Either don't capitalize "Curiously" (and prefix it with a "the"), or
> capitalize/title-case all four words.

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode60
src/elements.cc:60: MaybeObject** result) {
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:
> Why MaybeObject as result, if you always return an Object?

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode99
src/elements.cc:99: // identical except for specialization.
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> You specialize the only real function, so it's pretty non-identical.

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode180
src/elements.cc:180: inline static bool GetNumberDictionaryElement(
Yes, the NonStrictArgument accessor uses this.
On 2011/08/01 07:33:27, Rico wrote:
> Do we use this function outside this class, otherwise just make it private?

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode217
src/elements.cc:217: return NumberDictionary::cast(obj->element_dictionary());
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> 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.

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode264
src/elements.cc:264: class ConcreteElementsHandler : public ElementsHandler {
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> "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 :)

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

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode275
src/elements.cc:275: result);
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> 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?

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode310
src/elements.cc:310: } element_handlers;
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> Seems like a big static initializer. Might upset the Chrome people.

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode36
src/elements.h:36: // Abstract base class for handles that can operate on object
with differing
On 2011/08/01 07:33:27, Rico wrote:
> on object -> an object or objects

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode38
src/elements.h:38: class ElementsHandler {
My plan is to have this class house all of the element-related operations, so I
changed the name to Accessor. 

On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> What does "Handler" mean. Seems too generic to be meaningful.

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode48
src/elements.h:48: inline static ElementsHandler* ForKind(JSObject::ElementsKind
elements_kind) {
On 2011/08/01 10:18:23, Kevin Millikin wrote:
> No need for 'inline'.  Maybe the function name/signature should be something
> like SingletonFor(JSObject::ElementsKind).

Done.

http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode48
src/elements.h:48: inline static ElementsHandler* ForKind(JSObject::ElementsKind
elements_kind) {
singleton is not quite right, and I actually like the current naming. in the
code you get something like this:

ElementsAccessor::ForKind(...)

I am not sure that adding Singleton, or ObjectEnum or something of the like will
improve readability. But I'm happy to change it if you feel strongly.

On 2011/08/01 10:47:13, Lasse Reichstein wrote:
> We should probably not call it anything with "Singleton", since that's a less
> general (anti-)pattern (and we don't implement each class as a Singleton class
-
> which is a good thing!).
> This is more like an object-based enum (q.v. the Java enums, where,
> incidentally, "enum Foo" is implemented internally as "class Foo extends
> Enum<Foo>" :)

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h#newcode1604
src/objects-inl.h:1604: return get(index)->IsTheHole();
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> Is this really used so much that it needs this shorthand?
> I would have thought that in the cases where you need the check, you also need
> the value afterwards, if it isn't the hole.

Done.

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h#newcode1680
src/objects-inl.h:1680: return is_the_hole_nan(READ_DOUBLE_FIELD(this, offset));
Good question. I think not, since the (header) map + length are 64 bits
together.
 On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> Probably stupid question, but can this read be unaligned?

http://codereview.chromium.org/7527001/diff/15002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/objects.h#newcode74
src/objects.h:74: //       - FixedArray
On 2011/08/02 12:23:46, Lasse Reichstein wrote:
> I suddently remembered that this documentation is out-of-date now that there
is
> a FixedArrayBase. If possible, make it match reality :)

Done.

http://codereview.chromium.org/7527001/diff/15002/src/objects.h#newcode2070
src/objects.h:2070: class FixedArrayBase: public HeapObject {
That's a reasonable observation, but I think I'll leave that for another CL :-)
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
> Preferably this should be called FixedArray, as the generic superclass of all
> fixed-size arrays, and the original FixedArray should be ObjectFixedArray.
> I can see why it wasn't done, though, having done the same thing myself
> elsewhere :)

[Lasse Reichstein, 2011-08-02 13:59:58]

Point-comment only. I haven't checked the code yet, only read the responses (but
those seem good to me :).

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h#newcode1680
src/objects-inl.h:1680: return is_the_hole_nan(READ_DOUBLE_FIELD(this, offset));
I don't think that helps, since objects are pointer-aligned (v8globals.h declare
kObjectAlignment to be equal to kPointerSize).
It didn't pay off to align HeapNumbers, but it might for arrays of them. I.e.,
perhaps insert a one-word-filler before an unaligned double-array. But that
would also require GC to preserve alignment, so it's probably not a simple fix.

[Lasse Reichstein, 2011-08-03 10:35:38]

LGTM

http://codereview.chromium.org/7527001/diff/25004/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7527001/diff/25004/src/objects.cc#newcode11868
src/objects.cc:11868: // Single beak point.
Spotted a missing "r" in "break" here :)