Micro-optimize HashMap & String IPC decoding
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 21 Jul 2019 01:04:07 +0000 (01:04 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 21 Jul 2019 01:04:07 +0000 (01:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199967

Reviewed by Geoffrey Garen.

The legacy HashMap decoder (returning a boolean) was failing to WTFMove()
the key & value when calling HashMap::add(). The modern decoder (returning
an Optional) was properly using WTFMove(). Rewrite the legacy HashMap decoder
to call the modern one to reduce code duplication and to get this optimization.

Also, encode HashMap::size() as a uint32_t instead of a uint64_t since
HashMap::size() returns an 'unsigned int' type. Finally, update the modern
decoder to WTFMove(hashMap) when returning. Because the function returns an
Optional<HashMap> and not a HashMap, I do not believe we get return value
optimization (RVO).

Do similar changes to String IPC coders.

* Platform/IPC/ArgumentCoders.cpp:
(IPC::decodeStringText):
(IPC::ArgumentCoder<String>::decode):
* Platform/IPC/ArgumentCoders.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247672 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Platform/IPC/ArgumentCoders.cpp
Source/WebKit/Platform/IPC/ArgumentCoders.h

index 481c7c5..d171091 100644 (file)
@@ -1,3 +1,28 @@
+2019-07-20  Chris Dumez  <cdumez@apple.com>
+
+        Micro-optimize HashMap & String IPC decoding
+        https://bugs.webkit.org/show_bug.cgi?id=199967
+
+        Reviewed by Geoffrey Garen.
+
+        The legacy HashMap decoder (returning a boolean) was failing to WTFMove()
+        the key & value when calling HashMap::add(). The modern decoder (returning
+        an Optional) was properly using WTFMove(). Rewrite the legacy HashMap decoder
+        to call the modern one to reduce code duplication and to get this optimization.
+
+        Also, encode HashMap::size() as a uint32_t instead of a uint64_t since
+        HashMap::size() returns an 'unsigned int' type. Finally, update the modern
+        decoder to WTFMove(hashMap) when returning. Because the function returns an
+        Optional<HashMap> and not a HashMap, I do not believe we get return value
+        optimization (RVO).
+
+        Do similar changes to String IPC coders.
+
+        * Platform/IPC/ArgumentCoders.cpp:
+        (IPC::decodeStringText):
+        (IPC::ArgumentCoder<String>::decode):
+        * Platform/IPC/ArgumentCoders.h:
+
 2019-07-20  Alexander Mikhaylenko  <exalm7659@gmail.com>
 
         REGRESSION(r246033/r246496): [GTK] Kinetic scrolling doesn't work
index 56e6639..35b9130 100644 (file)
@@ -132,42 +132,20 @@ void ArgumentCoder<String>::encode(Encoder& encoder, const String& string)
 }
 
 template <typename CharacterType>
-static inline bool decodeStringText(Decoder& decoder, uint32_t length, String& result)
+static inline Optional<String> decodeStringText(Decoder& decoder, uint32_t length)
 {
     // Before allocating the string, make sure that the decoder buffer is big enough.
     if (!decoder.bufferIsLargeEnoughToContain<CharacterType>(length)) {
         decoder.markInvalid();
-        return false;
+        return WTF::nullopt;
     }
     
     CharacterType* buffer;
     String string = String::createUninitialized(length, buffer);
     if (!decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(buffer), length * sizeof(CharacterType), alignof(CharacterType)))
-        return false;
+        return WTF::nullopt;
     
-    result = string;
-    return true;    
-}
-
-bool ArgumentCoder<String>::decode(Decoder& decoder, String& result)
-{
-    uint32_t length;
-    if (!decoder.decode(length))
-        return false;
-
-    if (length == std::numeric_limits<uint32_t>::max()) {
-        // This is the null string.
-        result = String();
-        return true;
-    }
-
-    bool is8Bit;
-    if (!decoder.decode(is8Bit))
-        return false;
-
-    if (is8Bit)
-        return decodeStringText<LChar>(decoder, length, result);
-    return decodeStringText<UChar>(decoder, length, result);
+    return WTFMove(string);
 }
 
 Optional<String> ArgumentCoder<String>::decode(Decoder& decoder)
@@ -185,15 +163,19 @@ Optional<String> ArgumentCoder<String>::decode(Decoder& decoder)
     if (!decoder.decode(is8Bit))
         return WTF::nullopt;
     
-    String result;
-    if (is8Bit) {
-        if (!decodeStringText<LChar>(decoder, length, result))
-            return WTF::nullopt;
-        return result;
-    }
-    if (!decodeStringText<UChar>(decoder, length, result))
-        return WTF::nullopt;
-    return result;
+    if (is8Bit)
+        return decodeStringText<LChar>(decoder, length);
+    return decodeStringText<UChar>(decoder, length);
+}
+
+bool ArgumentCoder<String>::decode(Decoder& decoder, String& result)
+{
+    Optional<String> string;
+    decoder >> string;
+    if (!string)
+        return false;
+    result = WTFMove(*string);
+    return true;
 }
 
 void ArgumentCoder<SHA1::Digest>::encode(Encoder& encoder, const SHA1::Digest& digest)
index aa3fce6..a9302b4 100644 (file)
@@ -366,63 +366,47 @@ template<typename KeyArg, typename MappedArg, typename HashArg, typename KeyTrai
 
     static void encode(Encoder& encoder, const HashMapType& hashMap)
     {
-        encoder << static_cast<uint64_t>(hashMap.size());
+        encoder << static_cast<uint32_t>(hashMap.size());
         for (typename HashMapType::const_iterator it = hashMap.begin(), end = hashMap.end(); it != end; ++it)
             encoder << *it;
     }
 
-    static bool decode(Decoder& decoder, HashMapType& hashMap)
-    {
-        uint64_t hashMapSize;
-        if (!decoder.decode(hashMapSize))
-            return false;
-
-        HashMapType tempHashMap;
-        for (uint64_t i = 0; i < hashMapSize; ++i) {
-            KeyArg key;
-            MappedArg value;
-            if (!decoder.decode(key))
-                return false;
-            if (!decoder.decode(value))
-                return false;
-
-            if (!tempHashMap.add(key, value).isNewEntry) {
-                // The hash map already has the specified key, bail.
-                decoder.markInvalid();
-                return false;
-            }
-        }
-
-        hashMap.swap(tempHashMap);
-        return true;
-    }
-
     static Optional<HashMapType> decode(Decoder& decoder)
     {
-        uint64_t hashMapSize;
+        uint32_t hashMapSize;
         if (!decoder.decode(hashMapSize))
             return WTF::nullopt;
 
         HashMapType hashMap;
-        for (uint64_t i = 0; i < hashMapSize; ++i) {
+        for (uint32_t i = 0; i < hashMapSize; ++i) {
             Optional<KeyArg> key;
             decoder >> key;
-            if (!key)
+            if (UNLIKELY(!key))
                 return WTF::nullopt;
 
             Optional<MappedArg> value;
             decoder >> value;
-            if (!value)
+            if (UNLIKELY(!value))
                 return WTF::nullopt;
 
-            if (!hashMap.add(WTFMove(key.value()), WTFMove(value.value())).isNewEntry) {
+            if (UNLIKELY(!hashMap.add(WTFMove(*key), WTFMove(*value)).isNewEntry)) {
                 // The hash map already has the specified key, bail.
                 decoder.markInvalid();
                 return WTF::nullopt;
             }
         }
 
-        return hashMap;
+        return WTFMove(hashMap);
+    }
+
+    static bool decode(Decoder& decoder, HashMapType& hashMap)
+    {
+        Optional<HashMapType> tempHashMap;
+        decoder >> tempHashMap;
+        if (!tempHashMap)
+            return false;
+        hashMap.swap(*tempHashMap);
+        return true;
     }
 };