From b7cc30ae7e50cdb05a9ba4bf595fa79b110358bb Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Tue, 23 Jul 2024 18:52:29 +1200 Subject: [PATCH 1/9] Maintenance: Upgrade MEM_DWRITE_Q memory pool ... to a MEMPROXY class pool. Replacing memory allocate and destruct with C++ new/delete and in-class initialization. --- src/fs_io.cc | 42 +++++++++++------------------------------- src/fs_io.h | 15 ++++++++------- src/mem/forward.h | 1 - src/mem/old_api.cc | 1 - 4 files changed, 19 insertions(+), 40 deletions(-) diff --git a/src/fs_io.cc b/src/fs_io.cc index 609e03a43d6..b1df20b81fc 100644 --- a/src/fs_io.cc +++ b/src/fs_io.cc @@ -138,21 +138,11 @@ diskCombineWrites(_fde_disk *fdd) for (dwrite_q *q = fdd->write_q; q != nullptr; q = q->next) len += q->len - q->buf_offset; - dwrite_q *wq = (dwrite_q *)memAllocate(MEM_DWRITE_Q); - + auto *wq = new dwrite_q; wq->buf = (char *)xmalloc(len); + wq->free_func = cxx_xfree; // dwrite_q buffer xfree() - wq->len = 0; - - wq->buf_offset = 0; - - wq->next = nullptr; - - wq->free_func = cxx_xfree; - - while (fdd->write_q != nullptr) { - dwrite_q *q = fdd->write_q; - + while (auto *q = fdd->write_q) { len = q->len - q->buf_offset; memcpy(wq->buf + wq->len, q->buf + q->buf_offset, len); wq->len += len; @@ -161,7 +151,7 @@ diskCombineWrites(_fde_disk *fdd) if (q->free_func) q->free_func(q->buf); - memFree(q, MEM_DWRITE_Q); + delete q; }; fdd->write_q_tail = wq; @@ -178,11 +168,10 @@ diskHandleWrite(int fd, void *) fde *F = &fd_table[fd]; _fde_disk *fdd = &F->disk; - dwrite_q *q = fdd->write_q; int status = DISK_OK; bool do_close; - if (nullptr == q) + if (!fdd->write_q) return; debugs(6, 3, "diskHandleWrite: FD " << fd); @@ -246,23 +235,20 @@ diskHandleWrite(int fd, void *) * repeated write failures for the same FD because of * the queued data. */ - do { + while (auto *q = fdd->write_q) { fdd->write_q = q->next; if (q->free_func) q->free_func(q->buf); - if (q) { - memFree(q, MEM_DWRITE_Q); - q = nullptr; - } - } while ((q = fdd->write_q)); + delete q; + } } len = 0; } - if (q != nullptr) { + if (auto *q = fdd->write_q) { /* q might become NULL from write failure above */ q->buf_offset += len; @@ -280,10 +266,7 @@ diskHandleWrite(int fd, void *) if (q->free_func) q->free_func(q->buf); - if (q) { - memFree(q, MEM_DWRITE_Q); - q = nullptr; - } + delete q; } } @@ -331,17 +314,14 @@ file_write(int fd, void *handle_data, FREE * free_func) { - dwrite_q *wq = nullptr; fde *F = &fd_table[fd]; assert(fd >= 0); assert(F->flags.open); /* if we got here. Caller is eligible to write. */ - wq = (dwrite_q *)memAllocate(MEM_DWRITE_Q); + auto *wq = new dwrite_q; wq->file_offset = file_offset; wq->buf = (char *)ptr_to_buf; wq->len = len; - wq->buf_offset = 0; - wq->next = nullptr; wq->free_func = free_func; if (!F->disk.wrt_handle_data) { diff --git a/src/fs_io.h b/src/fs_io.h index 39645002daa..77a93e88151 100644 --- a/src/fs_io.h +++ b/src/fs_io.h @@ -30,16 +30,17 @@ class dread_ctrl void *client_data; }; -// POD class dwrite_q { + MEMPROXY_CLASS(dwrite_q); + public: - off_t file_offset; - char *buf; - size_t len; - size_t buf_offset; - dwrite_q *next; - FREE *free_func; + off_t file_offset = 0; + char *buf = nullptr; + size_t len = 0; + size_t buf_offset = 0; + dwrite_q *next = nullptr; + FREE *free_func = {}; // function to free 'buf' (if any) }; int file_open(const char *path, int mode); diff --git a/src/mem/forward.h b/src/mem/forward.h index 8a0b287fa14..0ec985739ee 100644 --- a/src/mem/forward.h +++ b/src/mem/forward.h @@ -52,7 +52,6 @@ typedef enum { MEM_32K_BUF, MEM_64K_BUF, MEM_DREAD_CTRL, - MEM_DWRITE_Q, MEM_MD5_DIGEST, MEM_MAX } mem_type; diff --git a/src/mem/old_api.cc b/src/mem/old_api.cc index 461be27d02d..d6fc8058fee 100644 --- a/src/mem/old_api.cc +++ b/src/mem/old_api.cc @@ -309,7 +309,6 @@ Mem::Init(void) memDataInit(MEM_64K_BUF, "64K Buffer", 65536, 10, false); // TODO: Carefully stop zeroing these objects memory and drop the doZero parameter memDataInit(MEM_DREAD_CTRL, "dread_ctrl", sizeof(dread_ctrl), 0, true); - memDataInit(MEM_DWRITE_Q, "dwrite_q", sizeof(dwrite_q), 0, true); memDataInit(MEM_MD5_DIGEST, "MD5 digest", SQUID_MD5_DIGEST_LENGTH, 0, true); GetPool(MEM_MD5_DIGEST)->setChunkSize(512 * 1024); From 50e4b3e9d1d6da2b74ba5b9064c12183f3f96918 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Tue, 24 Sep 2024 13:03:14 +1200 Subject: [PATCH 2/9] Reviewer required changes NO logic change to buffers yet. Just moving existing logic into constructor/destructor. --- src/fs_io.cc | 39 +++++++++++++++++++-------------------- src/fs_io.h | 6 +++++- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/fs_io.cc b/src/fs_io.cc index b1df20b81fc..148a8935dda 100644 --- a/src/fs_io.cc +++ b/src/fs_io.cc @@ -37,6 +37,23 @@ static void cxx_xfree(void *ptr) xfree(ptr); } +dwrite_q::dwrite_q(const size_t aSize, char *aBuffer, FREE *aFree) : + buf(aBuffer), + len(aSize), + free_func(aFree) +{ + if (!buf) { + buf = static_cast(xmalloc(len)); + free_func = cxx_xfree; // dwrite_q buffer xfree() + } +} + +dwrite_q::~dwrite_q() +{ + if (free_func) + free_func(buf); +} + /* * opens a disk file specified by 'path'. This function always * blocks! There is no callback. @@ -138,19 +155,12 @@ diskCombineWrites(_fde_disk *fdd) for (dwrite_q *q = fdd->write_q; q != nullptr; q = q->next) len += q->len - q->buf_offset; - auto *wq = new dwrite_q; - wq->buf = (char *)xmalloc(len); - wq->free_func = cxx_xfree; // dwrite_q buffer xfree() - + auto *wq = new dwrite_q(len); while (auto *q = fdd->write_q) { len = q->len - q->buf_offset; memcpy(wq->buf + wq->len, q->buf + q->buf_offset, len); wq->len += len; fdd->write_q = q->next; - - if (q->free_func) - q->free_func(q->buf); - delete q; }; @@ -237,10 +247,6 @@ diskHandleWrite(int fd, void *) */ while (auto *q = fdd->write_q) { fdd->write_q = q->next; - - if (q->free_func) - q->free_func(q->buf); - delete q; } } @@ -262,10 +268,6 @@ diskHandleWrite(int fd, void *) if (q->buf_offset == q->len) { /* complete write */ fdd->write_q = q->next; - - if (q->free_func) - q->free_func(q->buf); - delete q; } } @@ -318,11 +320,8 @@ file_write(int fd, assert(fd >= 0); assert(F->flags.open); /* if we got here. Caller is eligible to write. */ - auto *wq = new dwrite_q; + auto *wq = new dwrite_q(len, static_cast(const_cast(ptr_to_buf)), free_func); wq->file_offset = file_offset; - wq->buf = (char *)ptr_to_buf; - wq->len = len; - wq->free_func = free_func; if (!F->disk.wrt_handle_data) { F->disk.wrt_handle = handle; diff --git a/src/fs_io.h b/src/fs_io.h index 77a93e88151..c1493a7d398 100644 --- a/src/fs_io.h +++ b/src/fs_io.h @@ -33,8 +33,12 @@ class dread_ctrl class dwrite_q { MEMPROXY_CLASS(dwrite_q); - public: + dwrite_q() = default; + dwrite_q(const size_t, char * = nullptr, FREE * = nullptr); + dwrite_q(dwrite_q &&) = delete; // no copying or moving of any kind + ~dwrite_q(); + off_t file_offset = 0; char *buf = nullptr; size_t len = 0; From 0cd7a7c556e5b55989982cc7160a0016e3d89993 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 27 Sep 2024 14:00:04 +1200 Subject: [PATCH 3/9] Reviewer requested changes --- src/fs_io.cc | 10 +++++----- src/fs_io.h | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/fs_io.cc b/src/fs_io.cc index 148a8935dda..55d7048a23a 100644 --- a/src/fs_io.cc +++ b/src/fs_io.cc @@ -155,8 +155,8 @@ diskCombineWrites(_fde_disk *fdd) for (dwrite_q *q = fdd->write_q; q != nullptr; q = q->next) len += q->len - q->buf_offset; - auto *wq = new dwrite_q(len); - while (auto *q = fdd->write_q) { + const auto wq = new dwrite_q(len); + while (const auto q = fdd->write_q) { len = q->len - q->buf_offset; memcpy(wq->buf + wq->len, q->buf + q->buf_offset, len); wq->len += len; @@ -245,7 +245,7 @@ diskHandleWrite(int fd, void *) * repeated write failures for the same FD because of * the queued data. */ - while (auto *q = fdd->write_q) { + while (const auto q = fdd->write_q) { fdd->write_q = q->next; delete q; } @@ -254,7 +254,7 @@ diskHandleWrite(int fd, void *) len = 0; } - if (auto *q = fdd->write_q) { + if (const auto q = fdd->write_q) { /* q might become NULL from write failure above */ q->buf_offset += len; @@ -320,7 +320,7 @@ file_write(int fd, assert(fd >= 0); assert(F->flags.open); /* if we got here. Caller is eligible to write. */ - auto *wq = new dwrite_q(len, static_cast(const_cast(ptr_to_buf)), free_func); + const auto wq = new dwrite_q(len, static_cast(const_cast(ptr_to_buf)), free_func); wq->file_offset = file_offset; if (!F->disk.wrt_handle_data) { diff --git a/src/fs_io.h b/src/fs_io.h index c1493a7d398..75540743c5b 100644 --- a/src/fs_io.h +++ b/src/fs_io.h @@ -44,6 +44,8 @@ class dwrite_q size_t len = 0; size_t buf_offset = 0; dwrite_q *next = nullptr; + +private: FREE *free_func = {}; // function to free 'buf' (if any) }; From c5fbd5790a1cc8b7afb3fb4a2c8e5ee4b3d8399d Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 27 Sep 2024 14:02:45 +1200 Subject: [PATCH 4/9] Update src/fs_io.h Co-authored-by: Alex Rousskov --- src/fs_io.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/fs_io.h b/src/fs_io.h index 75540743c5b..2f3e35fc808 100644 --- a/src/fs_io.h +++ b/src/fs_io.h @@ -46,7 +46,10 @@ class dwrite_q dwrite_q *next = nullptr; private: - FREE *free_func = {}; // function to free 'buf' (if any) + +private: + /// when set, gets called upon object destruction to free buf + FREE *free_func = nullptr; }; int file_open(const char *path, int mode); From 9cb4fbbda29d9f91a60855736110f7196af4a186 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Thu, 10 Oct 2024 18:07:05 +1300 Subject: [PATCH 5/9] Separate capacity and length details Fixes regression in the write-merging case. --- src/fs_io.cc | 14 ++++++++------ src/fs_io.h | 9 ++++----- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/fs_io.cc b/src/fs_io.cc index 55d7048a23a..c8d31eec7e6 100644 --- a/src/fs_io.cc +++ b/src/fs_io.cc @@ -39,12 +39,14 @@ static void cxx_xfree(void *ptr) dwrite_q::dwrite_q(const size_t aSize, char *aBuffer, FREE *aFree) : buf(aBuffer), - len(aSize), + capacity(aSize), free_func(aFree) { if (!buf) { - buf = static_cast(xmalloc(len)); + buf = static_cast(xmalloc(aSize)); free_func = cxx_xfree; // dwrite_q buffer xfree() + } else { + len = aSize; } } @@ -150,14 +152,14 @@ diskCombineWrites(_fde_disk *fdd) */ if (fdd->write_q != nullptr && fdd->write_q->next != nullptr) { - int len = 0; + size_t wantCapacity = 0; for (dwrite_q *q = fdd->write_q; q != nullptr; q = q->next) - len += q->len - q->buf_offset; + wantCapacity += q->len - q->buf_offset; - const auto wq = new dwrite_q(len); + const auto wq = new dwrite_q(wantCapacity); while (const auto q = fdd->write_q) { - len = q->len - q->buf_offset; + const auto len = q->len - q->buf_offset; memcpy(wq->buf + wq->len, q->buf + q->buf_offset, len); wq->len += len; fdd->write_q = q->next; diff --git a/src/fs_io.h b/src/fs_io.h index 2f3e35fc808..5f73b8ff2b9 100644 --- a/src/fs_io.h +++ b/src/fs_io.h @@ -34,20 +34,19 @@ class dwrite_q { MEMPROXY_CLASS(dwrite_q); public: - dwrite_q() = default; - dwrite_q(const size_t, char * = nullptr, FREE * = nullptr); + dwrite_q(const size_t wantCapacity) : dwrite_q(wantCapacity, nullptr, nullptr) {} + dwrite_q(const size_t hasLength, char *, FREE *); dwrite_q(dwrite_q &&) = delete; // no copying or moving of any kind ~dwrite_q(); off_t file_offset = 0; char *buf = nullptr; - size_t len = 0; + size_t len = 0; ///< length of content in buf size_t buf_offset = 0; dwrite_q *next = nullptr; private: - -private: + size_t capacity = 0; ///< allocation size of buf /// when set, gets called upon object destruction to free buf FREE *free_func = nullptr; }; From 80f6294a32c8d7294f952af1dfa83042ea28ca67 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 11 Oct 2024 05:57:44 +1300 Subject: [PATCH 6/9] Update src/fs_io.cc Co-authored-by: Alex Rousskov --- src/fs_io.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fs_io.cc b/src/fs_io.cc index c8d31eec7e6..827890e4153 100644 --- a/src/fs_io.cc +++ b/src/fs_io.cc @@ -155,7 +155,7 @@ diskCombineWrites(_fde_disk *fdd) size_t wantCapacity = 0; for (dwrite_q *q = fdd->write_q; q != nullptr; q = q->next) - wantCapacity += q->len - q->buf_offset; + wantCapacity += q->len - q->buf_offset; // XXX: might overflow const auto wq = new dwrite_q(wantCapacity); while (const auto q = fdd->write_q) { From 1321718e827a0b3b559b2bceed754fae48bec87a Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 11 Oct 2024 05:58:46 +1300 Subject: [PATCH 7/9] Update src/fs_io.cc Co-authored-by: Alex Rousskov --- src/fs_io.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fs_io.cc b/src/fs_io.cc index 827890e4153..bc8124bd01e 100644 --- a/src/fs_io.cc +++ b/src/fs_io.cc @@ -37,7 +37,7 @@ static void cxx_xfree(void *ptr) xfree(ptr); } -dwrite_q::dwrite_q(const size_t aSize, char *aBuffer, FREE *aFree) : +dwrite_q::dwrite_q(const size_t aSize, char * const aBuffer, FREE * const aFree): buf(aBuffer), capacity(aSize), free_func(aFree) From a15a2c3e34df665860458bf4a64b16ef2380f2d2 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sat, 12 Oct 2024 00:05:08 +1300 Subject: [PATCH 8/9] Update src/fs_io.cc Co-authored-by: Alex Rousskov --- src/fs_io.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fs_io.cc b/src/fs_io.cc index bc8124bd01e..d723f059c85 100644 --- a/src/fs_io.cc +++ b/src/fs_io.cc @@ -42,6 +42,7 @@ dwrite_q::dwrite_q(const size_t aSize, char * const aBuffer, FREE * const aFree) capacity(aSize), free_func(aFree) { + assert(buf || !free_func); if (!buf) { buf = static_cast(xmalloc(aSize)); free_func = cxx_xfree; // dwrite_q buffer xfree() From 82f92ce8d06a4067073b878d5a2d5b1419511c40 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sat, 19 Oct 2024 22:44:05 +1300 Subject: [PATCH 9/9] Update src/fs_io.h Co-authored-by: Alex Rousskov --- src/fs_io.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fs_io.h b/src/fs_io.h index 5f73b8ff2b9..b2b3ff02d80 100644 --- a/src/fs_io.h +++ b/src/fs_io.h @@ -35,7 +35,7 @@ class dwrite_q MEMPROXY_CLASS(dwrite_q); public: dwrite_q(const size_t wantCapacity) : dwrite_q(wantCapacity, nullptr, nullptr) {} - dwrite_q(const size_t hasLength, char *, FREE *); + dwrite_q(size_t, char *, FREE *); dwrite_q(dwrite_q &&) = delete; // no copying or moving of any kind ~dwrite_q();