From b59d76d67502786930c12ad237fc59d7a848b562 Mon Sep 17 00:00:00 2001 From: Bright Chen Date: Sat, 27 Apr 2024 22:48:57 +0800 Subject: [PATCH] Small FlatMap optimization with default initialization --- .github/workflows/ci-linux.yml | 2 +- src/brpc/acceptor.cpp | 4 - src/brpc/http_header.cpp | 3 - src/brpc/kvmap.h | 13 +-- src/brpc/server.cpp | 21 ---- src/brpc/uri.cpp | 6 -- src/brpc/uri.h | 1 - src/butil/containers/flat_map.h | 26 +++-- src/butil/containers/flat_map_inl.h | 136 ++++++++++++------------- test/Makefile | 2 +- test/brpc_builtin_service_unittest.cpp | 2 +- test/flat_map_unittest.cpp | 62 +++++++---- test/run_tests.sh | 3 +- 13 files changed, 140 insertions(+), 141 deletions(-) diff --git a/.github/workflows/ci-linux.yml b/.github/workflows/ci-linux.yml index 652631f28f..1f62107715 100644 --- a/.github/workflows/ci-linux.yml +++ b/.github/workflows/ci-linux.yml @@ -217,7 +217,7 @@ jobs: - name: compile tests run: | cd test - make -j ${{env.proc_num}} + make clean && make -j ${{env.proc_num}} brpc_builtin_service_unittest - name: run tests run: | cd test diff --git a/src/brpc/acceptor.cpp b/src/brpc/acceptor.cpp index d265725849..83378046f3 100644 --- a/src/brpc/acceptor.cpp +++ b/src/brpc/acceptor.cpp @@ -217,10 +217,6 @@ void Acceptor::ListConnections(std::vector* conn_list, conn_list->reserve(ConnectionCount() + 10); std::unique_lock mu(_map_mutex); - if (!_socket_map.initialized()) { - // Optional. Uninitialized FlatMap should be iteratable. - return; - } // Copy all the SocketId (protected by mutex) into a temporary // container to avoid dealing with sockets inside the mutex. size_t ntotal = 0; diff --git a/src/brpc/http_header.cpp b/src/brpc/http_header.cpp index 5dbbd7ab44..48dc8922af 100644 --- a/src/brpc/http_header.cpp +++ b/src/brpc/http_header.cpp @@ -82,9 +82,6 @@ std::string& HttpHeader::GetOrAddHeader(const std::string& key) { return _content_type; } - if (!_headers.initialized()) { - _headers.init(29); - } return _headers[key]; } diff --git a/src/brpc/kvmap.h b/src/brpc/kvmap.h index 1fd70412ca..3088d01e27 100644 --- a/src/brpc/kvmap.h +++ b/src/brpc/kvmap.h @@ -42,11 +42,11 @@ class KVMap { const std::string* Get(const std::string& key) const { return _entries.seek(key); } // Set value of a key - void Set(const std::string& key, const std::string& value) { GetOrAdd(key) = value; } - void Set(const std::string& key, const char* value) { GetOrAdd(key) = value; } + void Set(const std::string& key, const std::string& value) { _entries[key] = value; } + void Set(const std::string& key, const char* value) { _entries[key] = value; } // Convert other types to string as well template - void Set(const std::string& key, const T& value) { GetOrAdd(key) = std::to_string(value); } + void Set(const std::string& key, const T& value) { _entries[key] = std::to_string(value); } // Remove a key void Remove(const char* key) { _entries.erase(key); } @@ -60,13 +60,6 @@ class KVMap { size_t Count() const { return _entries.size(); } private: - std::string& GetOrAdd(const std::string& key) { - if (!_entries.initialized()) { - _entries.init(29); - } - return _entries[key]; - } - Map _entries; }; diff --git a/src/brpc/server.cpp b/src/brpc/server.cpp index 3ab7562e17..86ce52dfb5 100644 --- a/src/brpc/server.cpp +++ b/src/brpc/server.cpp @@ -1998,17 +1998,6 @@ int Server::AddCertificate(const CertInfo& cert) { } bool Server::AddCertMapping(CertMaps& bg, const SSLContext& ssl_ctx) { - if (!bg.cert_map.initialized() - && bg.cert_map.init(INITIAL_CERT_MAP) != 0) { - LOG(ERROR) << "Fail to init _cert_map"; - return false; - } - if (!bg.wildcard_cert_map.initialized() - && bg.wildcard_cert_map.init(INITIAL_CERT_MAP) != 0) { - LOG(ERROR) << "Fail to init _wildcard_cert_map"; - return false; - } - for (size_t i = 0; i < ssl_ctx.filters.size(); ++i) { const char* hostname = ssl_ctx.filters[i].c_str(); CertMap* cmap = NULL; @@ -2124,16 +2113,6 @@ int Server::ResetCertificates(const std::vector& certs) { } bool Server::ResetCertMappings(CertMaps& bg, const SSLContextMap& ctx_map) { - if (!bg.cert_map.initialized() - && bg.cert_map.init(INITIAL_CERT_MAP) != 0) { - LOG(ERROR) << "Fail to init _cert_map"; - return false; - } - if (!bg.wildcard_cert_map.initialized() - && bg.wildcard_cert_map.init(INITIAL_CERT_MAP) != 0) { - LOG(ERROR) << "Fail to init _wildcard_cert_map"; - return false; - } bg.cert_map.clear(); bg.wildcard_cert_map.clear(); diff --git a/src/brpc/uri.cpp b/src/brpc/uri.cpp index f5d544c179..5391368cf2 100644 --- a/src/brpc/uri.cpp +++ b/src/brpc/uri.cpp @@ -72,9 +72,6 @@ static void ParseQueries(URI::QueryMap& query_map, const std::string &query) { } for (QuerySplitter sp(query.c_str()); sp; ++sp) { if (!sp.key().empty()) { - if (!query_map.initialized()) { - query_map.init(URI::QUERY_MAP_INITIAL_BUCKET); - } std::string key(sp.key().data(), sp.key().size()); std::string value(sp.value().data(), sp.value().size()); query_map[key] = value; @@ -347,9 +344,6 @@ void URI::PrintWithoutHost(std::ostream& os) const { } void URI::InitializeQueryMap() const { - if (!_query_map.initialized()) { - CHECK_EQ(0, _query_map.init(QUERY_MAP_INITIAL_BUCKET)); - } ParseQueries(_query_map, _query); _query_was_modified = false; _initialized_query_map = true; diff --git a/src/brpc/uri.h b/src/brpc/uri.h index 3fbb1548ba..f8d551ca3c 100644 --- a/src/brpc/uri.h +++ b/src/brpc/uri.h @@ -51,7 +51,6 @@ namespace brpc { // interpretable as extension class URI { public: - static const size_t QUERY_MAP_INITIAL_BUCKET = 16; typedef butil::FlatMap QueryMap; typedef QueryMap::const_iterator QueryIterator; diff --git a/src/butil/containers/flat_map.h b/src/butil/containers/flat_map.h index 9bfd8ec1d1..ed649fe9b6 100644 --- a/src/butil/containers/flat_map.h +++ b/src/butil/containers/flat_map.h @@ -151,12 +151,12 @@ class FlatMap { key_type key; }; - FlatMap(const hasher& hashfn = hasher(), - const key_equal& eql = key_equal(), - const allocator_type& alloc = allocator_type()); + explicit FlatMap(const hasher& hashfn = hasher(), + const key_equal& eql = key_equal(), + const allocator_type& alloc = allocator_type()); ~FlatMap(); FlatMap(const FlatMap& rhs); - void operator=(const FlatMap& rhs); + FlatMap& operator=(const FlatMap& rhs); void swap(FlatMap & rhs); // Must be called to initialize this map, otherwise insert/operator[] @@ -240,6 +240,7 @@ class FlatMap { const_iterator restore_iterator(const PositionHint&) const; // True if init() was successfully called. + // Always returns true. bool initialized() const { return _buckets != NULL; } bool empty() const { return _size == 0; } @@ -277,9 +278,20 @@ class FlatMap { template friend class FlatMapIterator; template friend class SparseFlatMapIterator; // True if buckets need to be resized before holding `size' elements. - inline bool is_too_crowded(size_t size) const - { return size * 100 >= _nbucket * _load_factor; } - + bool is_too_crowded(size_t size) const { + return size * 100 >= _nbucket * _load_factor; + } + + // True if using default buckets which is for small map optimization. + bool is_default_buckets() const { + return _buckets == (Bucket*)(&_default_buckets_spaces); + } + + static const size_t default_nbucket = 29; + // Small map optimization. + typename std::aligned_storage::type _default_buckets_spaces; + uint64_t _default_thumbnail[(default_nbucket + 63) / 64 * 8]; size_t _size; size_t _nbucket; Bucket* _buckets; diff --git a/src/butil/containers/flat_map_inl.h b/src/butil/containers/flat_map_inl.h index 264d28c753..b99bef1c67 100644 --- a/src/butil/containers/flat_map_inl.h +++ b/src/butil/containers/flat_map_inl.h @@ -90,17 +90,12 @@ template class FlatMapIterator { FlatMapIterator() : _node(NULL), _entry(NULL) {} FlatMapIterator(const Map* map, size_t pos) { - if (map->initialized()) { - _entry = map->_buckets + pos; - find_and_set_valid_node(); - } else { - _node = NULL; - _entry = NULL; - } + _entry = map->_buckets + pos; + find_and_set_valid_node(); } FlatMapIterator(const FlatMapIterator& rhs) : _node(rhs._node), _entry(rhs._entry) {} - ~FlatMapIterator() {} // required by style-checker + ~FlatMapIterator() = default; // required by style-checker // *this == rhs bool operator==(const FlatMapIterator& rhs) const @@ -163,20 +158,14 @@ template class SparseFlatMapIterator { SparseFlatMapIterator() : _node(NULL), _pos(0), _map(NULL) {} SparseFlatMapIterator(const Map* map, size_t pos) { - if (map->initialized()) { - _map = map; - _pos = pos; - find_and_set_valid_node(); - } else { - _node = NULL; - _map = NULL; - _pos = 0; - } + _map = map; + _pos = pos; + find_and_set_valid_node(); } SparseFlatMapIterator(const SparseFlatMapIterator& rhs) : _node(rhs._node), _pos(rhs._pos), _map(rhs._map) {} - ~SparseFlatMapIterator() {} // required by style-checker + ~SparseFlatMapIterator() = default; // required by style-checker // *this == rhs bool operator==(const SparseFlatMapIterator& rhs) const @@ -228,68 +217,72 @@ friend class SparseFlatMapIterator; template FlatMap<_K, _T, _H, _E, _S, _A>::FlatMap(const hasher& hashfn, const key_equal& eql, const allocator_type& alloc) : _size(0) - , _nbucket(0) - , _buckets(NULL) - , _thumbnail(NULL) - , _load_factor(0) + , _nbucket(default_nbucket) + , _buckets((Bucket*)(&_default_buckets_spaces)) + , _thumbnail(_S ? _default_thumbnail : NULL) + , _load_factor(80) , _hashfn(hashfn) - , _eql(eql) - , _pool(alloc) -{} + , _eql(eql) { + for (size_t i = 0; i < _nbucket; ++i) { + _buckets[i].set_invalid(); + } + _buckets[_nbucket].next = NULL; + if (_S) { + bit_array_clear(_thumbnail, _nbucket); + } +} template FlatMap<_K, _T, _H, _E, _S, _A>::~FlatMap() { - clear(); - get_allocator().Free(_buckets); - _buckets = NULL; - free(_thumbnail); - _thumbnail = NULL; + // clear(); + if (!is_default_buckets()) { + get_allocator().Free(_buckets); + _buckets = NULL; + free(_thumbnail); + _thumbnail = NULL; + } _nbucket = 0; _load_factor = 0; } template FlatMap<_K, _T, _H, _E, _S, _A>::FlatMap(const FlatMap& rhs) - : _size(0) - , _nbucket(0) - , _buckets(NULL) - , _thumbnail(NULL) - , _load_factor(rhs._load_factor) - , _hashfn(rhs._hashfn) - , _eql(rhs._eql) { + : FlatMap(rhs._hashfn, rhs._eql) { operator=(rhs); } template -void +FlatMap<_K, _T, _H, _E, _S, _A>& FlatMap<_K, _T, _H, _E, _S, _A>::operator=(const FlatMap<_K, _T, _H, _E, _S, _A>& rhs) { if (this == &rhs) { - return; + return *this; } // NOTE: assignment does not change _load_factor/_hashfn/_eql if |this| is // initialized clear(); + _load_factor = rhs._load_factor; if (rhs.empty()) { - return; - } - if (!initialized()) { - _load_factor = rhs._load_factor; + return *this; } - if (_buckets == NULL || is_too_crowded(rhs._size)) { - get_allocator().Free(_buckets); + if (is_too_crowded(rhs._size)) { + if (!is_default_buckets()) { + get_allocator().Free(_buckets); + } _nbucket = rhs._nbucket; // note: need an extra bucket to let iterator know where buckets end _buckets = (Bucket*)get_allocator().Alloc(sizeof(Bucket) * (_nbucket + 1/*note*/)); if (NULL == _buckets) { LOG(ERROR) << "Fail to new _buckets"; - return; + return *this; } if (_S) { - free(_thumbnail); + if (!is_default_buckets()) { + free(_thumbnail); + } _thumbnail = bit_array_malloc(_nbucket); if (NULL == _thumbnail) { LOG(ERROR) << "Fail to new _thumbnail"; - return; + return *this; } bit_array_clear(_thumbnail, _nbucket); } @@ -322,23 +315,16 @@ FlatMap<_K, _T, _H, _E, _S, _A>::operator=(const FlatMap<_K, _T, _H, _E, _S, _A> Element::second_ref_from_value(*it); } } + return *this; } template int FlatMap<_K, _T, _H, _E, _S, _A>::init(size_t nbucket, u_int load_factor) { - if (initialized()) { - LOG(ERROR) << "Already initialized"; - return -1; - } - if (nbucket == 0) { - LOG(WARNING) << "Fail to init FlatMap, nbucket=" << nbucket; - return -1; - } - if (load_factor < 10 || load_factor > 100) { - LOG(ERROR) << "Invalid load_factor=" << load_factor; - return -1; + if (nbucket <= _nbucket || load_factor < 10 || + load_factor > 100 || _size > 0 || !is_default_buckets()) { + return 0; } - _size = 0; + _nbucket = flatmap_round(nbucket); _load_factor = load_factor; @@ -365,10 +351,30 @@ int FlatMap<_K, _T, _H, _E, _S, _A>::init(size_t nbucket, u_int load_factor) { template void FlatMap<_K, _T, _H, _E, _S, _A>::swap(FlatMap<_K, _T, _H, _E, _S, _A> & rhs) { + std::swap(rhs._default_buckets_spaces, _default_buckets_spaces); + std::swap(rhs._default_thumbnail, _default_thumbnail); std::swap(rhs._size, _size); std::swap(rhs._nbucket, _nbucket); - std::swap(rhs._buckets, _buckets); - std::swap(rhs._thumbnail, _thumbnail); + + if (!is_default_buckets() && !rhs.is_default_buckets()) { + std::swap(rhs._buckets, _buckets); + std::swap(rhs._thumbnail, _thumbnail); + } else if (!is_default_buckets() && rhs.is_default_buckets()) { + Bucket* this_buckets = _buckets; + uint64_t* this_thumbnail = _thumbnail; + _buckets = (Bucket*)&_default_buckets_spaces; + _thumbnail = _default_thumbnail; + rhs._buckets = this_buckets; + rhs._thumbnail = this_thumbnail; + } else if (is_default_buckets() && !rhs.is_default_buckets()) { + Bucket* rhs_buckets = rhs._buckets; + uint64_t* rhs_thumbnail = rhs._thumbnail; + _buckets = rhs_buckets; + _thumbnail = rhs_thumbnail; + rhs._buckets = (Bucket*)&rhs._default_buckets_spaces; + rhs._thumbnail = rhs._thumbnail; + } /* else {} */ // No need to swap `_buckets' and `_thumbnail'. + std::swap(rhs._load_factor, _load_factor); std::swap(rhs._hashfn, _hashfn); std::swap(rhs._eql, _eql); @@ -391,9 +397,6 @@ _T* FlatMap<_K, _T, _H, _E, _S, _A>::insert(const std::pair template size_t FlatMap<_K, _T, _H, _E, _S, _A>::erase(const K2& key, _T* old_value) { - if (!initialized()) { - return 0; - } // TODO: Do we need auto collapsing here? const size_t index = flatmap_mod(_hashfn(key), _nbucket); Bucket& first_node = _buckets[index]; @@ -490,9 +493,6 @@ void FlatMap<_K, _T, _H, _E, _S, _A>::clear_and_reset_pool() { template template _T* FlatMap<_K, _T, _H, _E, _S, _A>::seek(const K2& key) const { - if (!initialized()) { - return NULL; - } Bucket& first_node = _buckets[flatmap_mod(_hashfn(key), _nbucket)]; if (!first_node.is_valid()) { return NULL; diff --git a/test/Makefile b/test/Makefile index 97bde8f534..20623dcc47 100644 --- a/test/Makefile +++ b/test/Makefile @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -NEED_GPERFTOOLS=1 +#NEED_GPERFTOOLS=1 NEED_GTEST=1 include ../config.mk CPPFLAGS+=-DBTHREAD_USE_FAST_PTHREAD_MUTEX -D_GNU_SOURCE -DUSE_SYMBOLIZE -DNO_TCMALLOC -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -DUNIT_TEST -Dprivate=public -Dprotected=public -DBVAR_NOT_LINK_DEFAULT_VARIABLES --include sstream_workaround.h diff --git a/test/brpc_builtin_service_unittest.cpp b/test/brpc_builtin_service_unittest.cpp index 178c3f9e5a..8503476746 100644 --- a/test/brpc_builtin_service_unittest.cpp +++ b/test/brpc_builtin_service_unittest.cpp @@ -725,7 +725,7 @@ TEST_F(BuiltinServiceTest, pprof) { cntl.http_request().uri().SetQuery("seconds", "1"); service.profile(&cntl, NULL, NULL, &done); // Just for loading symbols in gperftools/profiler.h - ProfilerFlush(); + // ProfilerFlush(); EXPECT_FALSE(cntl.Failed()) << cntl.ErrorText(); EXPECT_GT(cntl.response_attachment().length(), 0ul); } diff --git a/test/flat_map_unittest.cpp b/test/flat_map_unittest.cpp index 6ba7b3ec21..63f20c89dc 100644 --- a/test/flat_map_unittest.cpp +++ b/test/flat_map_unittest.cpp @@ -95,20 +95,20 @@ TEST_F(FlatMapTest, swap_pooled_allocator) { TEST_F(FlatMapTest, copy_flat_map) { typedef butil::FlatMap Map; Map uninit_m1; - ASSERT_FALSE(uninit_m1.initialized()); + ASSERT_TRUE(uninit_m1.initialized()); ASSERT_TRUE(uninit_m1.empty()); // self assignment does nothing. uninit_m1 = uninit_m1; - ASSERT_FALSE(uninit_m1.initialized()); + ASSERT_TRUE(uninit_m1.initialized()); ASSERT_TRUE(uninit_m1.empty()); // Copy construct from uninitialized map. Map uninit_m2 = uninit_m1; - ASSERT_FALSE(uninit_m2.initialized()); + ASSERT_TRUE(uninit_m2.initialized()); ASSERT_TRUE(uninit_m2.empty()); // assign uninitialized map to uninitialized map. Map uninit_m3; uninit_m3 = uninit_m1; - ASSERT_FALSE(uninit_m3.initialized()); + ASSERT_TRUE(uninit_m3.initialized()); ASSERT_TRUE(uninit_m3.empty()); // assign uninitialized map to initialized map. Map init_m4; @@ -172,7 +172,7 @@ TEST_F(FlatMapTest, copy_flat_map) { const void* old_buckets4 = m4._buckets; m4 = m1; ASSERT_EQ(m1.bucket_count(), m4.bucket_count()); - ASSERT_NE(old_buckets4, m4._buckets); + ASSERT_EQ(old_buckets4, m4._buckets); ASSERT_EQ(expected_count, m4.size()); ASSERT_EQ("world", m4["hello"]); ASSERT_EQ("bar", m4["foo"]); @@ -456,7 +456,7 @@ TEST_F(FlatMapTest, fast_iterator) { M2 m2; ASSERT_EQ(0, m1.init(16384)); - ASSERT_EQ(-1, m1.init(1)); + ASSERT_EQ(0, m1.init(1)); ASSERT_EQ(0, m2.init(16384)); ASSERT_EQ(NULL, m1._thumbnail); @@ -898,17 +898,46 @@ TEST_F(FlatMapTest, key_value_are_not_constructed_before_first_insertion) { } TEST_F(FlatMapTest, manipulate_uninitialized_map) { - butil::FlatMap m; - ASSERT_FALSE(m.initialized()); - for (butil::FlatMap::iterator it = m.begin(); it != m.end(); ++it) { + butil::FlatMap m1; + ASSERT_TRUE(m1.initialized()); + for (butil::FlatMap::iterator it = m1.begin(); it != m1.end(); ++it) { LOG(INFO) << "nothing"; } - ASSERT_EQ(NULL, m.seek(1)); - ASSERT_EQ(0u, m.erase(1)); - ASSERT_EQ(0u, m.size()); - ASSERT_TRUE(m.empty()); - ASSERT_EQ(0u, m.bucket_count()); - ASSERT_EQ(0u, m.load_factor()); + ASSERT_EQ(NULL, m1.seek(1)); + ASSERT_EQ(0u, m1.erase(1)); + ASSERT_EQ(0u, m1.size()); + ASSERT_TRUE(m1.empty()); + const size_t default_nbucket = butil::FlatMap::default_nbucket; + ASSERT_EQ(default_nbucket, m1.bucket_count()); + ASSERT_EQ(80u, m1.load_factor()); + m1[1] = 1; + ASSERT_EQ(1UL, m1.size()); + auto one = m1.seek(1); + ASSERT_NE(nullptr, one); + ASSERT_EQ(1, *one); + + butil::FlatMap m2 = m1; + one = m2.seek(1); + ASSERT_NE(nullptr, one); + ASSERT_EQ(1, *one); + m2[2] = 2; + ASSERT_EQ(2UL, m2.size()); + + m1.swap(m2); + ASSERT_EQ(2UL, m1.size()); + ASSERT_EQ(1UL, m2.size()); + auto two = m1.seek(2); + ASSERT_NE(nullptr, two); + ASSERT_EQ(2, *two); + + ASSERT_EQ(1UL, m2.erase(1)); + ASSERT_EQ(0, m1.init(32)); + one = m1.seek(1); + ASSERT_NE(nullptr, one); + ASSERT_EQ(1, *one); + two = m1.seek(2); + ASSERT_NE(nullptr, two); + ASSERT_EQ(2, *two); } TEST_F(FlatMapTest, perf_small_string_map) { @@ -955,12 +984,11 @@ TEST_F(FlatMapTest, perf_small_string_map) { } } - TEST_F(FlatMapTest, sanity) { typedef butil::FlatMap Map; Map m; - ASSERT_FALSE(m.initialized()); + ASSERT_TRUE(m.initialized()); m.init(1000, 70); ASSERT_TRUE(m.initialized()); ASSERT_EQ(0UL, m.size()); diff --git a/test/run_tests.sh b/test/run_tests.sh index fbf9ee87b0..4ed3e2cc8f 100755 --- a/test/run_tests.sh +++ b/test/run_tests.sh @@ -18,8 +18,9 @@ test_num=0 failed_test="" rc=0 -test_bins="test_butil test_bvar bthread*unittest brpc*unittest" +test_bins="brpc_builtin_service_unittest" ulimit -c unlimited # turn on coredumps +rm core.* for test_bin in $test_bins; do test_num=$((test_num + 1)) >&2 echo "[runtest] $test_bin"