Optimize freeMemorySpace() to not loop in vain #200

eduard-bagdasaryan · 2023-02-23T15:01:10Z

We should not perform this loop if the total number of idle memory pages
is less than required number of pages. An idle memory page in this scope
is an unlocked page in the local memory cache, belonging to a
replacement memory policy.

We should not perform this loop if the total number of idle memory pages is less than required number of pages.

Also update the global idle page counter when the number of entry's pages changes. Fortunately, there were only few mem_hdr methods (responsible for adding/removing memory nodes) to be adjusted. Also removed a couple of unused mem_hdr methods.

Also properly initialize mem_hdr::isIdle in constructor instead of supplying the correct value later. Also added more invariants (as assertions).

Also adjusted documentation.

We need to minimize the chance of calling mem_hdr::write() directly, i.e., bypassing updating mem_hdr::IdleNodes counter. The only caller now is StoreEntry::writeData(). Ideally, mem_hdr::write() should update IdleNodes itself, but but that would require passing StoreEntry pointer there (to check required conditions), which does not look as a good design. Also fixed test-suite tests.

src/stmem.h

rousskov

This looks promising, thank you, but more work is needed before we can commit to this solution.

rousskov · 2023-03-14T22:37:54Z

src/Store.h

@@ -319,6 +327,9 @@ class StoreEntry : public hash_link, public Packable
    /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it
    void lengthWentBad(const char *reason);

+    /// whether methods such as lock()/unlock() also require mem_hdr::IdleNodes counter updating
+    bool needsIdlePagesUpdating() const { return !lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL); }


This method implementation looks simple, but its hidden preconditions and/or implications are actually rather complex! This method cannot be correctly used/interpreted except in some very specific code places -- move its call a few lines up or down, and it will fail to work.

The method also exposes a potential terminology problem: Either we should not be checking ENTRY_SPECIAL (because ENTRY_SPECIAL is never idle) OR we are not really counting idle pages (i.e. we are counting "purge-able" pages).

For now, I suggest just inlining this method calls (and adjusting a bit to remove checks that are not necessary in each particular context). If, at the end of this PR, we still see reusable code that is more-or-less safe to call from various places, we will find a way to encapsulate it again.

I removed this method.

rousskov · 2023-03-14T22:52:24Z

src/store.cc

@@ -1543,7 +1561,7 @@ void
 StoreEntry::createMemObject()
 {
    assert(!mem_obj);
-    mem_obj = new MemObject();
+    mem_obj = new MemObject(locked());


Feels wrong that we do not care about ENTRY_SPECIAL here. If (current or future) code reaches this place with an unlocked ENTRY_SPECIAL, we will count "idle" pages we should not count... I hope we can refactor this PR to avoid this problem, but, for now, let's just add an XXX:

Suggested change

mem_obj = new MemObject(locked());

mem_obj = new MemObject(locked()); // XXX: May be unlocked ENTRY_SPECIAL

I removed this constructor argument. After modifying the condition used for making pages idle (i.e., it should be in a policy) I added an assertion into the MemObject constructor.

src/MemStore.cc

rousskov · 2023-03-14T23:01:55Z

src/stmem.cc

+void
+mem_hdr::setIdleness(const bool idle)
+{
+    assert(idle != isIdle);


If this code survives, I suggest relaxing it -- doing nothing when idleness does not change. It may even simplify some callers!

Removed this assertion.

src/stmem.cc

rousskov · 2023-03-14T23:07:08Z

src/store.cc

+    const auto oldSize = dataHdr.size();
+    assert(dataHdr.write(writeBuffer));
+    if (needsIdlePagesUpdating())
+        dataHdr.updateIdleNodes(oldSize);


Why do we need to call this explicitly here? In other words, why cannot dataHdr.write() adjust the idle page counter as needed? Do we not trust its isIdle state??

Removed this call.

These are unlocked pages that can be purged by the removal policy, i.e., by RemovalPurgeWalker.

eduard-bagdasaryan

Done at 2340315.

eduard-bagdasaryan · 2023-03-20T12:19:22Z

src/store.cc

+    const auto oldSize = dataHdr.size();
+    assert(dataHdr.write(writeBuffer));
+    if (needsIdlePagesUpdating())
+        dataHdr.updateIdleNodes(oldSize);


Removed this call.

eduard-bagdasaryan · 2023-03-20T12:23:03Z

src/store.cc

@@ -1543,7 +1561,7 @@ void
 StoreEntry::createMemObject()
 {
    assert(!mem_obj);
-    mem_obj = new MemObject();
+    mem_obj = new MemObject(locked());


I removed this constructor argument. After modifying the condition used for making pages idle (i.e., it should be in a policy) I added an assertion into the MemObject constructor.

src/stmem.h

eduard-bagdasaryan · 2023-03-20T12:24:50Z

src/stmem.cc

+void
+mem_hdr::setIdleness(const bool idle)
+{
+    assert(idle != isIdle);


Removed this assertion.

eduard-bagdasaryan · 2023-03-20T12:25:47Z

src/Store.h

@@ -319,6 +327,9 @@ class StoreEntry : public hash_link, public Packable
    /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it
    void lengthWentBad(const char *reason);

+    /// whether methods such as lock()/unlock() also require mem_hdr::IdleNodes counter updating
+    bool needsIdlePagesUpdating() const { return !lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL); }


I removed this method.

src/stmem.cc

src/MemStore.cc

rousskov · 2023-03-20T14:01:43Z

src/Store.h

@@ -67,6 +72,9 @@ class StoreEntry : public hash_link, public Packable
    bool isAccepting() const;
    size_t bytesWanted(Range<size_t> const aRange, bool ignoreDelayPool = false) const;

+    /// whether a non-completed (STORE_PENDING) entry is being written to the local memory store
+    bool isLocalWriter() const { return locked() && isAccepting(); }


AFAICT, the implementation of this new method does not match its name/description. Even without collapsed forwarding going on, a locked() isAccepting() entry may be "being written" by another worker, right? Locked() is true for virtually every entry "in use" by a worker, and isAccepting() is roughly equivalent to STORE_PENDING, which is true for entries written by any worker IIRC.

This does not necessarily mean the old code using this method condition was wrong -- that condition evaluation could have been placed in the right context, making it correct. I did not check.

The method also appears to rely on a very dangerous assumption that it is only called for STORE_PENDING entries. I think that is a method description defect though -- isLocked() checks STORE_PENDING rather than assuming that the method was called for STORE_PENDING entries.

Please remove this used-twice method. If the above notes did not change your opinion on the condition applicability in the new code, then, in the new caller context, just add an extra assert() or Assure() for locked(). It will also help distinguish the failing condition if the new assertion is triggered.

entry may be "being written" by another worker, right?

Right (i.e. ,written to a shared store), but then copied to by another worker and then written to a local store. In this sense, any STORE_PENDING is probably a 'local writer' and it looks 'too broad' definition. I removed this method.

rousskov · 2023-03-20T14:12:02Z

src/stmem.cc

+    if (newSize == oldSize)
+        return;


Nitpick: Please move these two lines up -- we do not need delta for this special noise-reduction(?) case.

rousskov · 2023-03-20T14:15:04Z

src/stmem.cc

+        return;
+    const auto oldCount = ReplPolicyIdleNodesCount;
+    if (newSize > oldSize) {
+        ReplPolicyIdleNodesCount += delta;


Missing overflow protection. Please use one of the SquidMath.h "natural" functions to protect.

Ok, but is there a possibility that the total number of pages (to say noting of idle pages) can exceed size_t maximum value?

rousskov · 2023-03-20T14:16:05Z

src/stmem.cc

+        ReplPolicyIdleNodesCount += delta;
+    } else {
+        assert(ReplPolicyIdleNodesCount >= delta);
+        ReplPolicyIdleNodesCount -= delta;


Missing underflow protection. Please use one of the SquidMath.h "natural" functions to protect, adding one if necessary.

I am not sure that 'underflow' is applicable here. How can it happen? We checked that A >= B where both operands are size_t (an unsigned integer type) and then calculate A-B - we must get a size_t C >=0.

rousskov · 2023-03-20T14:23:41Z

src/stmem.cc

+        ReplPolicyIdleNodesCount += size();
+    else {
+        assert(ReplPolicyIdleNodesCount >= size());
+        ReplPolicyIdleNodesCount -= size();


If possible, please reuse or merge with updateIdleNodes(). Refactor that method a little to be more reusable, as needed (e.g., giving it a second/newSize parameter?).

I was thinking to call one from another - but preconditions in these two methods a different - updateIdleNodes() is called only for idle nodes). I introduced a static helper method to avoid duplication.

rousskov · 2023-03-20T14:46:13Z

src/store.cc

        }

        ++hot_obj_count; // TODO: maintain for the shared hot cache as well
    } else {
        if (EBIT_TEST(flags, ENTRY_SPECIAL)) {
            debugs(20, 4, "not removing special " << *this << " from policy");
        } else {
-            mem_policy->Remove(mem_policy, this, &mem_obj->repl);
-            debugs(20, 4, "removed " << *this);
+            if (hasReplPolicy()) {


I think we should drop this new condition and the corresponding new method: If we need to assert() that ENTRY_SPECIAL entries are not counted/tracked, we should do that explicitly somewhere (not here where we explicitly check for ENTRY_SPECIAL and mem_obj) rather than implicitly (and rather unexpectedly and dangerously!) in all hasReplPolicy() callers.

Ok, I dropped the method but I think we need a condition (at least for data_hdr.allowedToFreeWithReplPolicy() call): we should skip this call when entry is not in memory policy. On the other hand, mem_policy->Remove() can be called twice - it has a such a check inside.

rousskov · 2023-03-20T15:00:09Z

src/store/Controller.cc

@@ -534,6 +534,9 @@ Store::Controller::freeMemorySpace(const int bytesRequired)
    if (memoryCacheHasSpaceFor(pagesRequired))
        return;

+    if (mem_hdr::ReplPolicyIdleNodesCount < static_cast<size_t>(pagesRequired))


Please use Less() for this comparison to avoid casting and clarify what is going on here:

Suggested change

if (mem_hdr::ReplPolicyIdleNodesCount < static_cast<size_t>(pagesRequired))

// do not free anything if freeing everything freeable would not be enough

if (Less(mem_hdr::ReplPolicyIdleNodesCount, pagesRequired))

Also safe numeric operations with Less() and IncreaseSum().

eduard-bagdasaryan

Done at 7820d21.

eduard-bagdasaryan · 2023-03-21T11:54:57Z

src/Store.h

@@ -67,6 +72,9 @@ class StoreEntry : public hash_link, public Packable
    bool isAccepting() const;
    size_t bytesWanted(Range<size_t> const aRange, bool ignoreDelayPool = false) const;

+    /// whether a non-completed (STORE_PENDING) entry is being written to the local memory store
+    bool isLocalWriter() const { return locked() && isAccepting(); }


entry may be "being written" by another worker, right?

Right (i.e. ,written to a shared store), but then copied to by another worker and then written to a local store. In this sense, any STORE_PENDING is probably a 'local writer' and it looks 'too broad' definition. I removed this method.

eduard-bagdasaryan · 2023-03-21T12:53:27Z

src/stmem.cc

+        ReplPolicyIdleNodesCount += size();
+    else {
+        assert(ReplPolicyIdleNodesCount >= size());
+        ReplPolicyIdleNodesCount -= size();


I was thinking to call one from another - but preconditions in these two methods a different - updateIdleNodes() is called only for idle nodes). I introduced a static helper method to avoid duplication.

eduard-bagdasaryan · 2023-03-21T14:34:11Z

src/stmem.cc

+    if (newSize == oldSize)
+        return;


eduard-bagdasaryan · 2023-03-21T14:37:02Z

src/stmem.cc

+        return;
+    const auto oldCount = ReplPolicyIdleNodesCount;
+    if (newSize > oldSize) {
+        ReplPolicyIdleNodesCount += delta;


Ok, but is there a possibility that the total number of pages (to say noting of idle pages) can exceed size_t maximum value?

eduard-bagdasaryan · 2023-03-21T14:40:51Z

src/stmem.cc

+        ReplPolicyIdleNodesCount += delta;
+    } else {
+        assert(ReplPolicyIdleNodesCount >= delta);
+        ReplPolicyIdleNodesCount -= delta;


I am not sure that 'underflow' is applicable here. How can it happen? We checked that A >= B where both operands are size_t (an unsigned integer type) and then calculate A-B - we must get a size_t C >=0.

eduard-bagdasaryan · 2023-03-21T15:03:41Z

src/store.cc

        }

        ++hot_obj_count; // TODO: maintain for the shared hot cache as well
    } else {
        if (EBIT_TEST(flags, ENTRY_SPECIAL)) {
            debugs(20, 4, "not removing special " << *this << " from policy");
        } else {
-            mem_policy->Remove(mem_policy, this, &mem_obj->repl);
-            debugs(20, 4, "removed " << *this);
+            if (hasReplPolicy()) {


Ok, I dropped the method but I think we need a condition (at least for data_hdr.allowedToFreeWithReplPolicy() call): we should skip this call when entry is not in memory policy. On the other hand, mem_policy->Remove() can be called twice - it has a such a check inside.

rousskov · 2023-03-21T17:05:10Z

src/stmem.cc

+    const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize;
+    const auto oldCount = mem_hdr::ReplPolicyIdleNodesCount;
+    if (newSize > oldSize) {
+        mem_hdr::ReplPolicyIdleNodesCount = IncreaseSum(mem_hdr::ReplPolicyIdleNodesCount, delta).value();


is there a possibility that the total number of pages (to say noting of idle pages) can exceed size_t maximum value?

IMO, yes: Buggy code (elsewhere) may forget to decrease the total (in some cases) or may increase the total by more than the number of pages in mem_hdr.

We could assert that there are no overflows, but that requires approximately the same effort AFAICT.

Did the IncreaseSum() address your concern?

rousskov · 2023-03-21T17:08:44Z

src/stmem.cc

+        mem_hdr::ReplPolicyIdleNodesCount = IncreaseSum(mem_hdr::ReplPolicyIdleNodesCount, delta).value();
+    } else {
+        assert(mem_hdr::ReplPolicyIdleNodesCount >= delta);
+        mem_hdr::ReplPolicyIdleNodesCount -= delta;


I am not sure that 'underflow' is applicable here. How can it happen? We checked that A >= B where both operands are size_t (an unsigned integer type) and then calculate A-B - we must get a size_t C >=0.

Agreed. We are essentially asserting that there is no underflow, and I missed that fact.

rousskov · 2023-03-21T17:32:24Z

src/store.cc

        }

        ++hot_obj_count; // TODO: maintain for the shared hot cache as well
    } else {
        if (EBIT_TEST(flags, ENTRY_SPECIAL)) {
            debugs(20, 4, "not removing special " << *this << " from policy");
        } else {
+            if (mem_obj->repl.data)
+                mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);
            mem_policy->Remove(mem_policy, this, &mem_obj->repl);


I think we need a condition (at least for data_hdr.allowedToFreeWithReplPolicy() call): we should skip this call when entry is not in memory policy.

Why?

I assume that if this (used to be IN_MEMORY!) entry is not in mem_policy for some reason, then its removableByReplPolicy flag is already false and calling allowedToFreeWithReplPolicy(false) is harmless. In all other cases, that call will (also) do what it is supposed to do -- decrease the total counter because we are removing the entry from the replacement policy. What am I missing?

In other words, this particular "removal from policy" event alone is sufficient to make removableByReplPolicy false, and our code should express that idea. It is also good to show asymmetry with the true case above it: It takes several events to make removableByReplPolicy true but only one event to make it false.

Why?

The intention was that if we do something with policy, we need to check that the policy exists (i.e., is configured). However you are right that we can call this allowedToFreeWithReplPolicy() even if the policy has been never configured - it will do nothing in this case.

rousskov · 2023-03-21T17:40:02Z

src/store.cc

+            if (!locked())
+                mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);


I suggest using allowedToFreeWithReplPolicy() "OK to call twice with the same value" convenience/safety feature and avoiding questions about the else case:

Suggested change

if (!locked())

mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);

// only idle entries can be freed by the replacement policy

mem_obj->data_hdr.allowedToFreeWithReplPolicy(!locked());

Alternatively, we should document what is going on in that (somewhat expected or should-be-common) else case:

Suggested change

if (!locked())

mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);

if (!locked())

mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);

// else the replacement policy cannot free a locked entry

I prefer the simplicity and clarity of the first variant, despite the extra function calls it causes. If those extra function calls are a concern, we should refactor allowedToFreeWithReplPolicy() declaration/implementation a little, to allow the compiler to inline the no-changes check into callers.

I applied the same logic in another allowedToFreeWithReplPolicy(true) change request, without an explanation.

I applied the first suggestion.

rousskov · 2023-03-21T17:56:42Z

src/store.cc

@@ -1949,6 +1968,13 @@ StoreEntry::checkDisk() const
    }
 }

+/// whether the entry was added to a memory replacement policy
+bool
+StoreEntry::hasReplPolicy() const


Remove as unused?

rousskov · 2023-03-21T18:00:59Z

src/store.cc

+StoreEntry::writeData(StoreIOBuffer writeBuffer)
+{
+    assert(mem_obj);
+    debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);


If this method survives, to simplify, reduce maintenance costs, and provide more info:

Suggested change

debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);

debugs(20, 6, writeBuffer);

I removed writeData() method.

rousskov · 2023-03-21T18:10:03Z

src/store.cc

+    assert(mem_obj);
+    debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);
+    auto &dataHdr = mem_obj->data_hdr;
+    assert (dataHdr.endOffset() || writeBuffer.offset == 0);


This is a new assertion (i.e. official code does not check this), right? It seems misplaced because it checks data_hdr-specific logic. Should not it go inside the mem_hdr::write() method instead? It is also rather weak and awkwardly phrased. I wonder whether this PR is the right place to add this assertion... In fact, should this PR add this method at all?? I suspect this method is no longer needed in this PR. If so, please remove instead of polishing.

This assertion was moved here from (removed) MemObject::write()). After our recent changes, I decided to remove writeData() at all and restore MemObject::write(). If we decide to refactor/eliminate the number off these write() methods scattered across several objects, let's do it separately.

rousskov · 2023-03-21T18:13:40Z

src/store.cc

+    if (!lock_count && mem_obj && mem_obj->repl.data)
+        mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);


Let's simplify/streamline this as well:

Suggested change

if (!lock_count && mem_obj && mem_obj->repl.data)

mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);

if (mem_obj)

mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);

Please also move this code right after the lock is incremented, to clarify the intent/precondition/cause.

rousskov · 2023-03-21T18:16:57Z

src/store.cc

+    if (mem_obj && mem_obj->repl.data)
+        mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);


Suggested change

if (mem_obj && mem_obj->repl.data)

mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);

if (mem_obj)

mem_obj->data_hdr.allowedToFreeWithReplPolicy(mem_obj->repl.data);

rousskov · 2023-03-21T18:21:07Z

src/store.cc

@@ -755,14 +760,14 @@ StoreEntry::write (StoreIOBuffer writeBuffer)
 {
    assert(mem_obj != nullptr);
    /* This assert will change when we teach the store to update */
-    assert(store_status == STORE_PENDING);


Can we keep this assertion? It feels like it was correct or, at the very least, should not be removed in this PR.

Restored: this was first moved to isLocalWriter(), but then I forgot to undo after removing isLocalWriter().

Also restored MemObject::write() and undone other write()-related refactoring.

eduard-bagdasaryan

Done with this iteration at 6423c13.

eduard-bagdasaryan · 2023-03-22T11:36:53Z

src/stmem.cc

+    const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize;
+    const auto oldCount = mem_hdr::ReplPolicyIdleNodesCount;
+    if (newSize > oldSize) {
+        mem_hdr::ReplPolicyIdleNodesCount = IncreaseSum(mem_hdr::ReplPolicyIdleNodesCount, delta).value();


Did the IncreaseSum() address your concern?

eduard-bagdasaryan · 2023-03-22T12:08:54Z

src/store.cc

+    if (!lock_count && mem_obj && mem_obj->repl.data)
+        mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);


eduard-bagdasaryan · 2023-03-22T12:09:36Z

src/store.cc

+    if (mem_obj && mem_obj->repl.data)
+        mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);


eduard-bagdasaryan · 2023-03-22T12:14:04Z

src/store.cc

@@ -755,14 +760,14 @@ StoreEntry::write (StoreIOBuffer writeBuffer)
 {
    assert(mem_obj != nullptr);
    /* This assert will change when we teach the store to update */
-    assert(store_status == STORE_PENDING);


Restored: this was first moved to isLocalWriter(), but then I forgot to undo after removing isLocalWriter().

eduard-bagdasaryan · 2023-03-22T12:29:30Z

src/store.cc

+            if (!locked())
+                mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);


I applied the first suggestion.

eduard-bagdasaryan · 2023-03-22T12:43:22Z

src/store.cc

@@ -1557,7 +1576,7 @@ void
 StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod)
 {
    if (!mem_obj)
-        mem_obj = new MemObject();
+        createMemObject();


eduard-bagdasaryan · 2023-03-22T12:44:04Z

src/store.cc

@@ -1949,6 +1968,13 @@ StoreEntry::checkDisk() const
    }
 }

+/// whether the entry was added to a memory replacement policy
+bool
+StoreEntry::hasReplPolicy() const


eduard-bagdasaryan · 2023-03-22T12:54:24Z

src/store.cc

+    assert(mem_obj);
+    debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);
+    auto &dataHdr = mem_obj->data_hdr;
+    assert (dataHdr.endOffset() || writeBuffer.offset == 0);


This assertion was moved here from (removed) MemObject::write()). After our recent changes, I decided to remove writeData() at all and restore MemObject::write(). If we decide to refactor/eliminate the number off these write() methods scattered across several objects, let's do it separately.

eduard-bagdasaryan · 2023-03-22T12:54:42Z

src/store.cc

+    debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);
+    auto &dataHdr = mem_obj->data_hdr;
+    assert (dataHdr.endOffset() || writeBuffer.offset == 0);
+    assert(dataHdr.write(writeBuffer));


I removed writeData() method.

eduard-bagdasaryan · 2023-03-22T12:54:54Z

src/store.cc

+StoreEntry::writeData(StoreIOBuffer writeBuffer)
+{
+    assert(mem_obj);
+    debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);


I removed writeData() method.

eduard-bagdasaryan added 9 commits February 23, 2023 17:57

Optimize freeMemorySpace() to not loop in vain

8cd8de0

We should not perform this loop if the total number of idle memory pages is less than required number of pages.

Merged from master

2bc801d

Removed 'locked' parameter from write() methods

ac3cf7c

Addressed a TODO in stmem.cc

33a2c2c

Also properly initialize mem_hdr::isIdle in constructor instead of supplying the correct value later. Also added more invariants (as assertions).

Partially fixed unit tests

919a6df

Fixed the rest of unit tests

df95793

Reduced diff by moving IdleNodes to stmem.cc

b9d2033

Also adjusted documentation.

eduard-bagdasaryan commented Mar 14, 2023

View reviewed changes

src/stmem.h Outdated Show resolved Hide resolved

rousskov requested changes Mar 14, 2023

View reviewed changes

Adjusted the 'idle pages' scope

2340315

These are unlocked pages that can be purged by the removal policy, i.e., by RemovalPurgeWalker.

eduard-bagdasaryan commented Mar 20, 2023

View reviewed changes

rousskov requested changes Mar 20, 2023

View reviewed changes

eduard-bagdasaryan added 2 commits March 20, 2023 18:01

Fixed unit tests

ec5917e

Removed questionable (helper) methods and code duplication

7820d21

Also safe numeric operations with Less() and IncreaseSum().

eduard-bagdasaryan commented Mar 21, 2023

View reviewed changes

rousskov requested changes Mar 21, 2023

View reviewed changes

Simplified, removing the 'mem_obj has memory policy' checks

6423c13

Also restored MemObject::write() and undone other write()-related refactoring.

eduard-bagdasaryan commented Mar 22, 2023

View reviewed changes

	mem_obj = new MemObject(locked());
	mem_obj = new MemObject(locked()); // XXX: May be unlocked ENTRY_SPECIAL

	if (mem_hdr::ReplPolicyIdleNodesCount < static_cast<size_t>(pagesRequired))
	// do not free anything if freeing everything freeable would not be enough
	if (Less(mem_hdr::ReplPolicyIdleNodesCount, pagesRequired))

		if (!locked())
		mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);

	debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);
	debugs(20, 6, writeBuffer);

		if (!lock_count && mem_obj && mem_obj->repl.data)
		mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);

		if (mem_obj && mem_obj->repl.data)
		mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);

Optimize freeMemorySpace() to not loop in vain #200

Are you sure you want to change the base?

Optimize freeMemorySpace() to not loop in vain #200

Conversation

eduard-bagdasaryan commented Feb 23, 2023 • edited Loading

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan commented Feb 23, 2023 •

edited

Loading