From 42b502299d428786938f90363cbfb62ae768cadb Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 8 Jan 2020 11:39:29 -0500 Subject: [PATCH] Change regcache list to be an hash map The regcache objects created for threads are stored in a linked list, regcache::current_regcache. This may result in bad performance when debugging a lot of threads, as GDB needs to walk the linked list to fetch a regcache given a ptid. This patch replaces the linked list with an std::unordered_map, indexed by (ptid, arch). The lookups, in get_thread_arch_aspace_regcache, should become faster when the number of threads grow. With a small number of threads, it will probably be a bit slower to do an hash map lookup than to walk a few linked list nodes, but it should not be perceptible. The function registers_changed_ptid deletes all regcaches related to a given ptid (there could be multiple, with different arches). Since the hash map is indexed by (ptid, arch), we can't do a simple lookup, so we fall back on iterating on all the hash map values. It should be the same complexity as what we have today. This function is called when resuming execution, so it would be nice to find a more efficient way to do this. Similarly, the function regcache_thread_ptid_changed is called when a thread changes ptid, so it needs to look up all regcaches for a given ptid. This one also iterates on all the hash map values. However, it is not called often (mostly when creating an inferior, on some specific platforms), so it shouldn't be a problem. --- gdb/gdbsupport/ptid.h | 13 ++++++ gdb/regcache.c | 103 ++++++++++++++++++++++++++++++++---------- gdb/regcache.h | 3 -- gdb/thread-map.h | 9 ---- 4 files changed, 91 insertions(+), 37 deletions(-) diff --git a/gdb/gdbsupport/ptid.h b/gdb/gdbsupport/ptid.h index f5625a6138..d00a0964bb 100644 --- a/gdb/gdbsupport/ptid.h +++ b/gdb/gdbsupport/ptid.h @@ -32,6 +32,8 @@ thread_stratum target that might want to sit on top. */ +#include + class ptid_t { public: @@ -143,6 +145,17 @@ private: long m_tid; }; +struct hash_ptid +{ + size_t operator() (const ptid_t &ptid) const + { + std::hash long_hash; + return (long_hash (ptid.pid ()) + + long_hash (ptid.lwp ()) + + long_hash (ptid.tid ())); + } +}; + /* The null or zero ptid, often used to indicate no process. */ extern const ptid_t null_ptid; diff --git a/gdb/regcache.c b/gdb/regcache.c index 6912b14d06..adffa415e5 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -317,21 +317,58 @@ reg_buffer::assert_regnum (int regnum) const recording if the register values have been changed (eg. by the user). Therefore all registers must be written back to the target when appropriate. */ -std::forward_list regcache::current_regcache; + +/* Key for the hash map keeping the regcaches. */ + +struct ptid_arch +{ + ptid_arch (ptid_t ptid, gdbarch *arch) + : ptid (ptid), arch (arch) + {} + + ptid_t ptid; + gdbarch *arch; + + bool operator== (const ptid_arch &other) const + { + return this->ptid == other.ptid && this->arch == other.arch; + } +}; + +/* Hash function for ptid_arch. */ + +struct hash_ptid_arch +{ + size_t operator() (const ptid_arch &val) const + { + hash_ptid h_ptid; + std::hash h_long; + return h_ptid (val.ptid) + h_long ((long) val.arch); + } +}; + +using ptid_arch_regcache_map = std::unordered_map; + +/* Hash map containing the regcaches. */ + +static ptid_arch_regcache_map the_regcaches; struct regcache * -get_thread_arch_aspace_regcache (ptid_t ptid, struct gdbarch *gdbarch, +get_thread_arch_aspace_regcache (ptid_t ptid, gdbarch *arch, struct address_space *aspace) { - for (const auto ®cache : regcache::current_regcache) - if (regcache->ptid () == ptid && regcache->arch () == gdbarch) - return regcache; - - regcache *new_regcache = new regcache (gdbarch, aspace); + /* Look up a regcache for this (ptid, arch). */ + ptid_arch key (ptid, arch); + auto it = the_regcaches.find (key); + if (it != the_regcaches.end ()) + return it->second; - regcache::current_regcache.push_front (new_regcache); + /* It does not exist, create it. */ + regcache *new_regcache = new regcache (arch, aspace); new_regcache->set_ptid (ptid); + the_regcaches[key] = new_regcache; + return new_regcache; } @@ -390,13 +427,32 @@ regcache_observer_target_changed (struct target_ops *target) /* Update global variables old ptids to hold NEW_PTID if they were holding OLD_PTID. */ -void -regcache::regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) +static void +regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) { - for (auto ®cache : regcache::current_regcache) + std::vector keys_to_update; + + /* Find all the regcaches to updates. */ + for (auto &pair : the_regcaches) { - if (regcache->ptid () == old_ptid) - regcache->set_ptid (new_ptid); + regcache *rc = pair.second; + if (rc->ptid () == old_ptid) + keys_to_update.push_back (pair.first); + } + + for (const ptid_arch &old_key : keys_to_update) + { + /* Get the regcache, delete the hash map entry. */ + auto it = the_regcaches.find (old_key); + gdb_assert (it != the_regcaches.end ()); + regcache *rc = it->second; + + the_regcaches.erase (it); + + /* Insert the regcache back, with an updated key. */ + ptid_arch new_key (new_ptid, rc->arch ()); + rc->set_ptid (new_ptid); + the_regcaches[new_key] = rc; } } @@ -414,18 +470,17 @@ regcache::regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) void registers_changed_ptid (ptid_t ptid) { - for (auto oit = regcache::current_regcache.before_begin (), - it = std::next (oit); - it != regcache::current_regcache.end (); - ) + for (auto iter = the_regcaches.begin (); iter != the_regcaches.end (); ) { - if ((*it)->ptid ().matches (ptid)) + regcache *rc = iter->second; + + if (rc->ptid ().matches (ptid)) { - delete *it; - it = regcache::current_regcache.erase_after (oit); + delete iter->second; + iter = the_regcaches.erase (iter); } else - oit = it++; + ++iter; } if (current_thread_ptid.matches (ptid)) @@ -1395,8 +1450,7 @@ public: static size_t current_regcache_size () { - return std::distance (regcache::current_regcache.begin (), - regcache::current_regcache.end ()); + return the_regcaches.size (); } }; @@ -1803,8 +1857,7 @@ _initialize_regcache (void) = gdbarch_data_register_post_init (init_regcache_descr); gdb::observers::target_changed.attach (regcache_observer_target_changed); - gdb::observers::thread_ptid_changed.attach - (regcache::regcache_thread_ptid_changed); + gdb::observers::thread_ptid_changed.attach (regcache_thread_ptid_changed); add_com ("flushregs", class_maintenance, reg_flush_command, _("Force gdb to flush its register cache (maintainer command).")); diff --git a/gdb/regcache.h b/gdb/regcache.h index f7d4bc60f6..f8af38cf97 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -389,12 +389,9 @@ public: debug. */ void debug_print_register (const char *func, int regno); - static void regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid); protected: regcache (gdbarch *gdbarch, const address_space *aspace_); - static std::forward_list current_regcache; - private: /* Helper function for transfer_regset. Copies across a single register. */ diff --git a/gdb/thread-map.h b/gdb/thread-map.h index 2b41b7361c..44cccc16c4 100644 --- a/gdb/thread-map.h +++ b/gdb/thread-map.h @@ -25,15 +25,6 @@ struct thread_info; -struct hash_ptid -{ - size_t operator() (const ptid_t &ptid) const - { - std::hash long_hash; - return long_hash(ptid.pid()) + long_hash(ptid.lwp()) + long_hash(ptid.tid()); - } -}; - using ptid_thread_map = std::unordered_map; struct all_thread_map_range_iterator -- 2.34.1