From a83e58b8de80f39ff5a55442b6988137121fe830 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 26 Oct 2024 21:21:28 +0200 Subject: [PATCH] Solving potential thread collision issues upon Layout::update. --- src/db/db/dbLayout.cc | 64 +++++++++++++++++++++------ src/db/db/dbLayout.h | 31 +++++-------- src/db/db/dbLayoutStateModel.cc | 10 +++-- src/db/db/dbLayoutStateModel.h | 2 +- src/db/db/dbRecursiveShapeIterator.cc | 4 +- 5 files changed, 70 insertions(+), 41 deletions(-) diff --git a/src/db/db/dbLayout.cc b/src/db/db/dbLayout.cc index b69ca8d73..327d91415 100644 --- a/src/db/db/dbLayout.cc +++ b/src/db/db/dbLayout.cc @@ -1749,8 +1749,49 @@ Layout::end_top_cells () const return m_top_down_list.begin () + m_top_cells; } +void +Layout::start_changes () +{ + tl::MutexLocker locker (&lock ()); + ++m_invalid; +} + +void +Layout::end_changes () +{ + tl::MutexLocker locker (&lock ()); + if (m_invalid > 0) { + if (--m_invalid == 0) { + force_update_no_lock (); + } + } +} + +void +Layout::end_changes_no_update () +{ + tl::MutexLocker locker (&lock ()); + if (m_invalid > 0) { + --m_invalid; + } +} + void Layout::force_update () +{ + // NOTE: the assumption is that either one thread is writing or + // multiple threads are reading. Hence, we do not need to lock hier_dirty() or bboxes_dirty(). + // We still do double checking as another thread might do the update as well. + if (! hier_dirty () && ! bboxes_dirty ()) { + return; + } + + tl::MutexLocker locker (&lock ()); + force_update_no_lock (); +} + +void +Layout::force_update_no_lock () const { if (hier_dirty () || bboxes_dirty ()) { @@ -1776,22 +1817,17 @@ Layout::force_update () void Layout::update () const { - if (! under_construction () && (hier_dirty () || bboxes_dirty ())) { + // NOTE: the assumption is that either one thread is writing or + // multiple threads are reading. Hence, we do not need to lock hier_dirty() or bboxes_dirty(). + // We still do double checking as another thread might do the update as well. + if (under_construction () || (! hier_dirty () && ! bboxes_dirty ())) { + return; + } - try { - - m_invalid = std::numeric_limits::max (); // prevent recursion - - db::LayoutStateModel *state_model = const_cast ((const db::LayoutStateModel *) this); - state_model->update (); - - m_invalid = 0; - - } catch (...) { - m_invalid = 0; - throw; - } + tl::MutexLocker locker (&lock ()); + if (! under_construction ()) { + force_update_no_lock (); } } diff --git a/src/db/db/dbLayout.h b/src/db/db/dbLayout.h index f5b28cd0f..e7b3d21a3 100644 --- a/src/db/db/dbLayout.h +++ b/src/db/db/dbLayout.h @@ -624,7 +624,7 @@ public: * @brief Gets the lock for the layout object * This is a generic lock that can be used to lock modifications against multiple threads. */ - tl::Mutex &lock () + tl::Mutex &lock () const { return m_lock; } @@ -1791,34 +1791,18 @@ public: * an operation, the start/end_changes method pair does not * need to be called. */ - void start_changes () - { - ++m_invalid; - } + void start_changes (); /** * @brief Cancel the "in changes" state (see "start_changes") */ - void end_changes () - { - if (m_invalid > 0) { - --m_invalid; - if (! m_invalid) { - update (); - } - } - } + void end_changes (); /** * @brief Cancel the "in changes" state (see "start_changes") * This version does not force an update */ - void end_changes_no_update () - { - if (m_invalid > 0) { - --m_invalid; - } - } + void end_changes_no_update (); /** * @brief Tell if the layout object is under construction @@ -2168,7 +2152,7 @@ private: std::map m_meta_info_by_cell; std::string m_tech_name; - tl::Mutex m_lock; + mutable tl::Mutex m_lock; /** * @brief Sort the cells topologically @@ -2211,6 +2195,11 @@ private: * @brief Recovers a proxy without considering the library from context_info */ db::Cell *recover_proxy_no_lib (const LayoutOrCellContextInfo &context_info); + + /** + * @brief Does an update without checking under_construction() and no Mutex + */ + void force_update_no_lock () const; }; /** diff --git a/src/db/db/dbLayoutStateModel.cc b/src/db/db/dbLayoutStateModel.cc index 4bc02d822..80eecfd6c 100644 --- a/src/db/db/dbLayoutStateModel.cc +++ b/src/db/db/dbLayoutStateModel.cc @@ -29,13 +29,14 @@ namespace db { LayoutStateModel::LayoutStateModel (bool busy) - : m_hier_dirty (false), m_hier_generation_id (0), m_all_bboxes_dirty (false), m_busy (busy) + : m_hier_dirty (false), m_hier_generation_id (0), m_all_bboxes_dirty (false), m_some_bboxes_dirty (false), m_busy (busy) { // .. nothing yet .. } LayoutStateModel::LayoutStateModel (const LayoutStateModel &d) - : m_hier_dirty (d.m_hier_dirty), m_hier_generation_id (d.m_hier_generation_id), m_bboxes_dirty (d.m_bboxes_dirty), m_all_bboxes_dirty (d.m_all_bboxes_dirty), m_busy (d.m_busy) + : m_hier_dirty (d.m_hier_dirty), m_hier_generation_id (d.m_hier_generation_id), m_bboxes_dirty (d.m_bboxes_dirty), + m_all_bboxes_dirty (d.m_all_bboxes_dirty), m_some_bboxes_dirty (d.m_some_bboxes_dirty), m_busy (d.m_busy) { // .. nothing yet .. } @@ -47,6 +48,7 @@ LayoutStateModel::operator= (const LayoutStateModel &d) m_hier_generation_id = d.m_hier_generation_id; m_bboxes_dirty = d.m_bboxes_dirty; m_all_bboxes_dirty = d.m_all_bboxes_dirty; + m_some_bboxes_dirty = d.m_some_bboxes_dirty; m_busy = d.m_busy; return *this; } @@ -84,6 +86,7 @@ LayoutStateModel::invalidate_bboxes (unsigned int index) m_bboxes_dirty.resize (index + 1, false); } m_bboxes_dirty [index] = true; + m_some_bboxes_dirty = true; } } } @@ -91,7 +94,7 @@ LayoutStateModel::invalidate_bboxes (unsigned int index) bool LayoutStateModel::bboxes_dirty () const { - return ! m_bboxes_dirty.empty () || m_all_bboxes_dirty; + return m_some_bboxes_dirty || m_all_bboxes_dirty; } void @@ -100,6 +103,7 @@ LayoutStateModel::update () if (bboxes_dirty () || m_hier_dirty) { do_update (); m_bboxes_dirty.clear (); + m_some_bboxes_dirty = false; m_all_bboxes_dirty = false; m_hier_dirty = false; } diff --git a/src/db/db/dbLayoutStateModel.h b/src/db/db/dbLayoutStateModel.h index 281298b74..071d30eba 100644 --- a/src/db/db/dbLayoutStateModel.h +++ b/src/db/db/dbLayoutStateModel.h @@ -209,7 +209,7 @@ private: bool m_hier_dirty; size_t m_hier_generation_id; std::vector m_bboxes_dirty; - bool m_all_bboxes_dirty; + bool m_all_bboxes_dirty, m_some_bboxes_dirty; bool m_busy; void do_invalidate_hier (); diff --git a/src/db/db/dbRecursiveShapeIterator.cc b/src/db/db/dbRecursiveShapeIterator.cc index 1361983ca..27d86e2aa 100644 --- a/src/db/db/dbRecursiveShapeIterator.cc +++ b/src/db/db/dbRecursiveShapeIterator.cc @@ -703,8 +703,8 @@ RecursiveShapeIterator::next (RecursiveShapeReceiver *receiver) } if (at_end ()) { - // release the layout lock if at end - this way, the shape iterator can be - // held further, without blocking the layout. + // Take this opportunity the release the layout lock. + // This way, the shape iterator can be held further, without blocking the layout. m_locker = db::LayoutLocker (); }