This is an automated email from the ASF dual-hosted git repository.
jvanderzee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 3ea4d8ff63 Implement RAII for `PreservationTable` (#11825)
3ea4d8ff63 is described below
commit 3ea4d8ff639c354cc9090ff42368c3be367867af
Author: JosiahWI <[email protected]>
AuthorDate: Thu Nov 7 16:13:20 2024 -0600
Implement RAII for `PreservationTable` (#11825)
* Add a constructor to `PreservationTable`
* Delete `PreservationTable` copy constructors
* Implement moves for `PreservationTable`
* Add destructor to `PreservationTable`
---
src/iocore/cache/PreservationTable.cc | 37 ++++++++++++++++++++++++++++++
src/iocore/cache/PreservationTable.h | 23 ++++++++++++++++++-
src/iocore/cache/StripeSM.cc | 11 ++++-----
src/iocore/cache/unit_tests/test_Stripe.cc | 3 ---
4 files changed, 64 insertions(+), 10 deletions(-)
diff --git a/src/iocore/cache/PreservationTable.cc
b/src/iocore/cache/PreservationTable.cc
index c45deb2d95..572e9af74f 100644
--- a/src/iocore/cache/PreservationTable.cc
+++ b/src/iocore/cache/PreservationTable.cc
@@ -31,7 +31,12 @@
#include "tsutil/DbgCtl.h"
+#include "tscore/ink_assert.h"
+#include "tscore/ink_memory.h"
+#include "tscore/List.h"
+
#include <cinttypes>
+#include <cstring>
namespace
{
@@ -40,6 +45,38 @@ DbgCtl dbg_ctl_cache_evac{"cache_evac"};
} // namespace
+PreservationTable::PreservationTable(int size) : evacuate_size{(size /
EVACUATION_BUCKET_SIZE) + 2}
+{
+ ink_assert(size > 0);
+ int evac_len = this->evacuate_size * sizeof(DLL<EvacuationBlock>);
+ this->evacuate = static_cast<DLL<EvacuationBlock> *>(ats_malloc(evac_len));
+ memset(static_cast<void *>(this->evacuate), 0, evac_len);
+}
+
+PreservationTable::~PreservationTable()
+{
+ ats_free(this->evacuate);
+ this->evacuate = nullptr;
+}
+
+PreservationTable::PreservationTable(PreservationTable &&that)
+{
+ this->evacuate_size = that.evacuate_size;
+ this->evacuate = that.evacuate;
+ that.evacuate_size = 0;
+ that.evacuate = nullptr;
+}
+
+PreservationTable &
+PreservationTable::operator=(PreservationTable &&that)
+{
+ this->evacuate_size = that.evacuate_size;
+ this->evacuate = that.evacuate;
+ that.evacuate_size = 0;
+ that.evacuate = nullptr;
+ return *this;
+}
+
EvacuationBlock *
PreservationTable::find(Dir const &dir) const
{
diff --git a/src/iocore/cache/PreservationTable.h
b/src/iocore/cache/PreservationTable.h
index 7641456b45..acf762cce9 100644
--- a/src/iocore/cache/PreservationTable.h
+++ b/src/iocore/cache/PreservationTable.h
@@ -36,8 +36,8 @@
#include "tscore/ink_platform.h"
#include "tscore/List.h"
-#define EVACUATION_BUCKET_SIZE (2 * EVACUATION_SIZE) // 16MB
#define EVACUATION_SIZE (2 * AGG_SIZE) // 8MB
+#define EVACUATION_BUCKET_SIZE (2 * EVACUATION_SIZE) // 16MB
#define PIN_SCAN_EVERY 16 // scan every 1/16 of disk
#define dir_offset_evac_bucket(_o) (_o / (EVACUATION_BUCKET_SIZE /
CACHE_BLOCK_SIZE))
@@ -85,6 +85,9 @@ struct EvacuationBlock {
* This class is not safe for concurrent access. It should be protected
* by a lock.
*
+ * Destructing this object without first releasing all blocks and calling
+ * periodic_scan may result in memory leaks.
+ *
* @see Stripe
*/
class PreservationTable
@@ -99,6 +102,24 @@ public:
*/
DLL<EvacuationBlock> *evacuate{nullptr};
+ /**
+ * PreservationTable constructor
+ *
+ * This will abort the program if it is unable to allocate memory.
+ *
+ * @param size The total number of directory entries the table must
+ * be able to store. The size must be a positive number.
+ */
+ PreservationTable(int size);
+
+ ~PreservationTable();
+
+ PreservationTable(PreservationTable const &that) = delete;
+ PreservationTable &operator=(PreservationTable const &that) = delete;
+
+ PreservationTable(PreservationTable &&that);
+ PreservationTable &operator=(PreservationTable &&that);
+
/**
* Check whether the hash table may be indexed with the given offset.
*
diff --git a/src/iocore/cache/StripeSM.cc b/src/iocore/cache/StripeSM.cc
index d3c901ec59..d5899dbe29 100644
--- a/src/iocore/cache/StripeSM.cc
+++ b/src/iocore/cache/StripeSM.cc
@@ -109,13 +109,12 @@ struct StripeInitInfo {
}
};
-StripeSM::StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip) :
Continuation(new_ProxyMutex()), Stripe{disk, blocks, dir_skip}
+// This is weird: the len passed to the constructor for _preserved_dirs is
+// initialized in the superclasse's constructor. This is safe because the
+// superclass should always be initialized first.
+StripeSM::StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip)
+ : Continuation(new_ProxyMutex()), Stripe{disk, blocks, dir_skip},
_preserved_dirs{static_cast<int>(len)}
{
- this->_preserved_dirs.evacuate_size = static_cast<int>(len /
EVACUATION_BUCKET_SIZE) + 2;
- int evac_len = this->_preserved_dirs.evacuate_size *
sizeof(DLL<EvacuationBlock>);
- this->_preserved_dirs.evacuate = static_cast<DLL<EvacuationBlock>
*>(ats_malloc(evac_len));
- memset(static_cast<void *>(this->_preserved_dirs.evacuate), 0, evac_len);
-
open_dir.mutex = this->mutex;
SET_HANDLER(&StripeSM::aggWrite);
}
diff --git a/src/iocore/cache/unit_tests/test_Stripe.cc
b/src/iocore/cache/unit_tests/test_Stripe.cc
index 6c09b1af00..dd81dcc8bb 100644
--- a/src/iocore/cache/unit_tests/test_Stripe.cc
+++ b/src/iocore/cache/unit_tests/test_Stripe.cc
@@ -193,7 +193,6 @@ TEST_CASE("The behavior of StripeSM::add_writer.")
}
ats_free(stripe.raw_dir);
- ats_free(stripe.get_preserved_dirs().evacuate);
ats_free(stripe.path);
}
@@ -305,7 +304,6 @@ TEST_CASE("aggWrite behavior with f.evacuator unset")
}
ats_free(stripe.raw_dir);
- ats_free(stripe.get_preserved_dirs().evacuate);
ats_free(stripe.path);
}
@@ -405,6 +403,5 @@ TEST_CASE("aggWrite behavior with f.evacuator set")
delete[] source;
ats_free(stripe.raw_dir);
- ats_free(stripe.get_preserved_dirs().evacuate);
ats_free(stripe.path);
}
