Bug 115410 – Change HashTraits<RefPtr<P> >::PassOutType to PassRefPtr for better performance

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

SSIA.

[Mikhail Pozdnyakov]

Created attachment 200099 [details]
WIP

Do we want to take after HashTraits<OwnPtr<P> > and use std::nullptr_t for EmptyValueType?

[Mikhail Pozdnyakov]

Comment on attachment 200099 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=200099&action=review

> Source/WTF/wtf/HashTraits.h:145
> +        static PassRefPtr<P> passOut(const RefPtr<P>& value) { return value; }

that would be better to call release here, but that would break constness.

[Darin Adler]

Comment on attachment 200099 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=200099&action=review

>> Source/WTF/wtf/HashTraits.h:145
>> +        static PassRefPtr<P> passOut(const RefPtr<P>& value) { return value; }
> 
> that would be better to call release here, but that would break constness.

The release() is where the performance benefit comes from. So this is not useful to do without the release(). This should instead be:

    typedef PassRefPtr<P> PassOutType;
    static PassRefPtr<P> passOut(RefPtr<P>& value) { return value.release(); }

Could you test to see if that compiles on various platforms and what it does?

[Darin Adler]

(In reply to comment #1)
> Do we want to take after HashTraits<OwnPtr<P> > and use std::nullptr_t for EmptyValueType?

Yes, I think we will need to do that so that the take function compiles.

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 200099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200099&action=review
> 
> >> Source/WTF/wtf/HashTraits.h:145
> >> +        static PassRefPtr<P> passOut(const RefPtr<P>& value) { return value; }
> > 
> > that would be better to call release here, but that would break constness.
> 
> The release() is where the performance benefit comes from. So this is not useful to do without the release(). 
This should instead be:
> 
>     typedef PassRefPtr<P> PassOutType;
>     static PassRefPtr<P> passOut(RefPtr<P>& value) { return value.release(); }
> 
> Could you test to see if that compiles on various platforms and what it does?

in order to make it compile-able we will need to use std::nullptr_t as empty type, provide corresponding constructors to RefPtr/PassRefPtr and probably also add one more equal() method to hash translator.. but I can do it :)

[Mikhail Pozdnyakov]

> > The release() is where the performance benefit comes from. So this is not useful to do without the release(). 
> This should instead be:
> > 
> >     typedef PassRefPtr<P> PassOutType;
> >     static PassRefPtr<P> passOut(RefPtr<P>& value) { return value.release(); }
> > 
> > Could you test to see if that compiles on various platforms and what it does?
> 
> in order to make it compile-able we will need to use std::nullptr_t as empty type, provide corresponding constructors to RefPtr/PassRefPtr and probably also add one more equal() method to hash translator.. but I can do it :)

This is actually much more complex to support std::nullptr_t as empty type:
1) there is necessity to add "==" operators accepting nullptr into a lot of places.
2) having both constructors accepting raw pointer to 'T' and std::nullptr_t inside RefPtr/PassRefPtr brings ambiguousness to some calls.

So I would choose another way of solving the initial compiling issue:
having following method inside HashTraits<RefPtr<P> >

       static RefPtr<P>& emptyValue()
        {
            static RefPtr<P>& null = *(new RefPtr<P>);
            return null;
        }

[Mikhail Pozdnyakov]

Created attachment 200304 [details]
patch

[Darin Adler]

(In reply to comment #6)
> So I would choose another way of solving the initial compiling issue:
> having following method inside HashTraits<RefPtr<P> >
> 
>        static RefPtr<P>& emptyValue()
>         {
>             static RefPtr<P>& null = *(new RefPtr<P>);
>             return null;
>         }

This won’t be good for performance. Null needs to be more efficient than this. Please don’t do this.

[Darin Adler]

Comment on attachment 200304 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200304&action=review

If we are having trouble making nullptr work, maybe we can make a zero work? I’m sure there is some way to do this. I think you are learning now why I landed this with a FIXME rather than doing this task originally.

> Source/WTF/wtf/HashTraits.h:145
> +        static RefPtr<P>& emptyValue()
> +        {
> +            static RefPtr<P>& null = *(new RefPtr<P>);
> +            return null;
> +        }

I understand this is easier to get right than the alternative, but it’s not going to be efficient to fetch a global variable all the time. Since the whole point of this is to get better performance, we must not make this change!

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 200304 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200304&action=review
> 
> If we are having trouble making nullptr work, maybe we can make a zero work? 
think it's gonna work with '0'.

I’m sure there is some way to do this. I think you are learning now why I landed this with a FIXME rather than doing this task originally.
yeah I got it :)

[Mikhail Pozdnyakov]

Created attachment 200406 [details]
patch v2

Use '0' as empty value.

[Mikhail Pozdnyakov]

Comment on attachment 200406 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=200406&action=review

> Source/WTF/wtf/HashTraits.h:141
> +        static P* emptyValue() { return 0; }

it's probably worth adding "static const bool emptyValueIsZero = true;"

[Mikhail Pozdnyakov]

(In reply to comment #11)
> Created an attachment (id=200406) [details]
> patch v2
> 
> Use '0' as empty value.

An alternate solution could be the following:

template<typename P> struct HashTraits<RefPtr<P> > : SimpleClassHashTraits<RefPtr<P> > {
        ..........
        typedef PassRefPtr<P> PassOutType;
        static PassRefPtr<P> passOut(RefPtr<P>& value) { return value.release(); }
        static PassRefPtr<P> passOut(const RefPtr<P>& value) // Overload for temporary, must be used for empty value only.
        {
            ASSERT(!value);
            return 0;
        }
    };

what do you think?

[Darin Adler]

Comment on attachment 200406 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=200406&action=review

> Source/WTF/wtf/HashTable.h:304
>          template<typename T> static bool equal(const T& a, const T& b) { return HashFunctions::equal(a, b); }
> +        template<typename T, typename U> static bool equal(const T& a, const U& b) { return HashFunctions::equal(a, b); }

When we add the two-type equal function template, we should remove the old single-type function template. No need to keep both.

>> Source/WTF/wtf/HashTraits.h:141
>> +        static P* emptyValue() { return 0; }
> 
> it's probably worth adding "static const bool emptyValueIsZero = true;"

Doesn’t need to be added. SimpleClassHashTraits provides that.

> Source/WTF/wtf/HashTraits.h:149
> +        static PassRefPtr<P> passOut(P* value) { return value; }
>          // FIXME: We should consider changing PeekType to a raw pointer for better performance,

I suggest a blank line here so that PassOutType and PeekType are in separate paragraphs.

[Mikhail Pozdnyakov]

Committed r149602: <http://trac.webkit.org/changeset/149602>