SSIA.
Created attachment 200099 [details] WIP Do we want to take after HashTraits<OwnPtr<P> > and use std::nullptr_t for EmptyValueType?
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.
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?
(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.
(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 :)
> > 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; }
Created attachment 200304 [details] patch
(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.
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!
(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 :)
Created attachment 200406 [details] patch v2 Use '0' as empty value.
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;"
(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?
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.
Committed r149602: <http://trac.webkit.org/changeset/149602>