From b0eb7e3ea662525545e60fcb092272afaa1cddb2 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 | 116 +++++++++++++++++++++++++++++++----------- gdb/regcache.h | 3 -- gdb/thread-map.h | 9 ---- 4 files changed, 100 insertions(+), 41 deletions(-) diff --git a/gdb/gdbsupport/ptid.h b/gdb/gdbsupport/ptid.h index ef52d55512..232593af5d 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 2dfe8eaf5f..87d95010e3 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -318,26 +318,65 @@ 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 target_ptid_arch +{ + target_ptid_arch (process_stratum_target *target, ptid_t ptid, gdbarch *arch) + : target (target), ptid (ptid), arch (arch) + {} + + process_stratum_target *target; + ptid_t ptid; + gdbarch *arch; + + bool operator== (const target_ptid_arch &other) const + { + return (this->target == other.target + && this->ptid == other.ptid + && this->arch == other.arch); + } +}; + +/* Hash function for target_ptid_arch. */ + +struct hash_target_ptid_arch +{ + size_t operator() (const target_ptid_arch &val) const + { + hash_ptid h_ptid; + std::hash h_long; + return h_ptid (val.ptid) + h_long ((long) val.arch); + } +}; + +using target_ptid_arch_regcache_map + = std::unordered_map; + +/* Hash map containing the regcaches. */ + +static target_ptid_arch_regcache_map the_regcaches; struct regcache * get_thread_arch_aspace_regcache (process_stratum_target *target, - ptid_t ptid, struct gdbarch *gdbarch, + ptid_t ptid, gdbarch *arch, struct address_space *aspace) { gdb_assert (target != nullptr); - for (const auto ®cache : regcache::current_regcache) - if (regcache->target () == target - && regcache->ptid () == ptid - && regcache->arch () == gdbarch) - return regcache; - - regcache *new_regcache = new regcache (target, gdbarch, aspace); + /* Look up a regcache for this (target, ptid, arch). */ + target_ptid_arch key (target, 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 (target, arch, aspace); new_regcache->set_ptid (ptid); + the_regcaches[key] = new_regcache; + return new_regcache; } @@ -413,13 +452,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 target_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. */ + target_ptid_arch new_key (rc->target (), new_ptid, rc->arch ()); + rc->set_ptid (new_ptid); + the_regcaches[new_key] = rc; } } @@ -437,20 +495,22 @@ regcache::regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) void registers_changed_ptid (process_stratum_target *target, ptid_t ptid) { - for (auto oit = regcache::current_regcache.before_begin (), - it = std::next (oit); - it != regcache::current_regcache.end (); - ) + /* If we have a non-minus_one_ptid, we must have a non-NULL target. */ + if (ptid != minus_one_ptid) + gdb_assert (target != nullptr); + + for (auto iter = the_regcaches.begin (); iter != the_regcaches.end (); ) { - struct regcache *regcache = *it; - if ((target == nullptr || regcache->target () == target) - && regcache->ptid ().matches (ptid)) + regcache *rc = iter->second; + + if ((target == nullptr || rc->target () == target) + && rc->ptid ().matches (ptid)) { - delete regcache; - it = regcache::current_regcache.erase_after (oit); + delete iter->second; + iter = the_regcaches.erase (iter); } else - oit = it++; + ++iter; } if ((target == nullptr || current_thread_target == target) @@ -1423,8 +1483,7 @@ public: static size_t current_regcache_size () { - return std::distance (regcache::current_regcache.begin (), - regcache::current_regcache.end ()); + return the_regcaches.size (); } }; @@ -1861,8 +1920,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 b8561d714c..820088618d 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -397,13 +397,10 @@ 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 (process_stratum_target *target, 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