Optimization of HashTraits<RefPtr<P> >::emptyValue()

Optimization of HashTraits<RefPtr<P> >::emptyValue()

HashTraits<RefPtr<P> >::emptyValue() is better just to return zero pointer
rather than access static RefPtr instance, further HashTraits<RefPtr<P> >::passOut() can be overriden to accept raw pointer (for empty value).
This all should improve HashMap performance in case its value type is RefPtr.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150425

[Mikhail, 2013-05-06 17:32:30]

Hi, 

here is one more patch optimizing HashMap<.., RefPtr>.

BR,
Mikhail

[eseidel, 2013-05-06 18:38:59]

https://codereview.chromium.org/14877006/diff/1/Source/wtf/HashTraits.h
File Source/wtf/HashTraits.h (right):

https://codereview.chromium.org/14877006/diff/1/Source/wtf/HashTraits.h#newco...
Source/wtf/HashTraits.h:148: static PassRefPtr<P> passOut(P* value) { return
value; }
What does this do?  Is it safe?

[Mikhail, 2013-05-06 19:31:28]

On 2013/05/06 18:38:59, Eric Seidel (Google) wrote:
> https://codereview.chromium.org/14877006/diff/1/Source/wtf/HashTraits.h
> File Source/wtf/HashTraits.h (right):
> 
>
https://codereview.chromium.org/14877006/diff/1/Source/wtf/HashTraits.h#newco...
> Source/wtf/HashTraits.h:148: static PassRefPtr<P> passOut(P* value) { return
> value; }
> What does this do?  Is it safe?

This is overridden function for empty value, "value" here has to be 0. Pls refer
to HashMap implementation:
    HashMap<T, U, V, W, MappedTraits>::take(const KeyType& key)
    {
        iterator it = find(key);
        if (it == end())
            return MappedTraits::passOut(MappedTraits::emptyValue());
        MappedPassOutType result = MappedTraits::passOut(it->value);
        remove(it);
        return result;
    }

Same solution was accepted for WebKit wtf:
http://trac.webkit.org/changeset/149602

[eseidel, 2013-05-06 19:40:40]

lgtm

OK.  Referencing the WK Commit definitely makes the review easier. :)

[commit-bot: I haz the power, 2013-05-06 19:41:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/14877006/1

[commit-bot: I haz the power, 2013-05-06 19:41:19]

Failed to apply patch for Source/wtf/HashTraits.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/wtf/HashTraits.h
  Hunk #1 FAILED at 138.
  1 out of 1 hunk FAILED -- saving rejects to file Source/wtf/HashTraits.h.rej

Patch:       Source/wtf/HashTraits.h
Index: Source/wtf/HashTraits.h
diff --git a/Source/wtf/HashTraits.h b/Source/wtf/HashTraits.h
index
69df8b781a1447b8f1951c7f955e5c0490768d9d..fe4c12f60e6058309c83bbf58c357f92d939ee9c
100644
--- a/Source/wtf/HashTraits.h
+++ b/Source/wtf/HashTraits.h
@@ -138,17 +138,15 @@ namespace WTF {
     };
 
     template<typename P> struct HashTraits<RefPtr<P> > :
SimpleClassHashTraits<RefPtr<P> > {
-        static RefPtr<P>& emptyValue()
-        {
-            static RefPtr<P>& null = *(new RefPtr<P>);
-            return null;
-        }
+        static P* emptyValue() { return 0; }
 
         typedef PassRefPtr<P> PassInType;
         static void store(PassRefPtr<P> value, RefPtr<P>& storage) { storage =
value; }
 
         typedef PassRefPtr<P> PassOutType;
         static PassRefPtr<P> passOut(RefPtr<P>& value) { return
value.release(); }
+        static PassRefPtr<P> passOut(P* value) { return value; }
+
         // FIXME: We should consider changing PeekType to a raw pointer for
better performance,
         // but then callers won't need to call get; doing so will require
updating many call sites.
     };

[commit-bot: I haz the power, 2013-05-08 08:05:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/14877006/...

[commit-bot: I haz the power, 2013-05-08 09:21:15]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[kenneth.r.christiansen, 2013-05-08 10:02:36]

You should remove the BUG= from the description. Can you please do that?

[commit-bot: I haz the power, 2013-05-10 06:41:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/14877006/...

[commit-bot: I haz the power, 2013-05-10 12:03:54]

Retried try job too often on win_layout for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-13 14:23:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/14877006/...

[commit-bot: I haz the power, 2013-05-13 14:24:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/14877006/...

[commit-bot: I haz the power, 2013-05-13 15:03:00]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-15 09:35:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/14877006/...

[commit-bot: I haz the power, 2013-05-15 14:12:48]

Change committed as 150425