Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize freeMemorySpace() to not loop in vain #200

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 1 addition & 13 deletions src/MemObject.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ MemObject::MemObject()
ping_reply_callback = nullptr;
memset(&start_ping, 0, sizeof(start_ping));
reply_ = new HttpReply;
assert(!repl.data);
}

MemObject::~MemObject()
Expand Down Expand Up @@ -132,19 +133,6 @@ MemObject::replaceBaseReply(const HttpReplyPointer &r)
updatedReply_ = nullptr;
}

void
MemObject::write(const StoreIOBuffer &writeBuffer)
{
debugs(19, 6, "memWrite: offset " << writeBuffer.offset << " len " << writeBuffer.length);

/* We don't separate out mime headers yet, so ensure that the first
* write is at offset 0 - where they start
*/
assert (data_hdr.endOffset() || writeBuffer.offset == 0);

assert (data_hdr.write (writeBuffer));
}

void
MemObject::dump() const
{
Expand Down
1 change: 0 additions & 1 deletion src/MemObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ class MemObject
/// whether setUris() has been called
bool hasUris() const;

void write(const StoreIOBuffer &buf);
void unlinkRequest() { request = nullptr; }

/// HTTP response before 304 (Not Modified) updates
Expand Down
2 changes: 1 addition & 1 deletion src/MemStore.cc
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof)

// local memory stores both headers and body so copy regardless of pstate
const int64_t offBefore = e.mem_obj->endOffset();
assert(e.mem_obj->data_hdr.write(buf)); // from MemObject::write()
e.writeData(buf);
const int64_t offAfter = e.mem_obj->endOffset();
rousskov marked this conversation as resolved.
Show resolved Hide resolved
// expect to write the entire buf because StoreEntry::write() never fails
assert(offAfter >= 0 && offBefore <= offAfter &&
Expand Down
7 changes: 7 additions & 0 deletions src/Store.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ class StoreEntry : public hash_link, public Packable
/// \see MemObject::freshestReply()
const HttpReply *hasFreshestReply() const { return mem_obj ? &mem_obj->freshestReply() : nullptr; }

/// writes to local memory store
void writeData(StoreIOBuffer);

/// writeData() and calls awaiting handlers.
/// Should be called for non-completed (STORE_PENDING) entries only.
void write(StoreIOBuffer);

/** Check if the Store entry is empty
Expand Down Expand Up @@ -319,6 +324,8 @@ 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);

bool hasReplPolicy() const;

static Mem::Allocator *pool;

unsigned short lock_count; /* Assume < 65536! */
Expand Down
48 changes: 47 additions & 1 deletion src/stmem.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@
#include "HttpReply.h"
#include "mem_node.h"
#include "MemObject.h"
#include "SquidMath.h"
#include "stmem.h"

size_t mem_hdr::ReplPolicyIdleNodesCount = 0;

/*
* NodeGet() is called to get the data buffer to pass to storeIOWrite().
* By setting the write_pending flag here we are assuming that there
Expand Down Expand Up @@ -58,7 +61,9 @@ mem_hdr::endOffset () const
void
mem_hdr::freeContent()
{
const auto initialNodes = size();
nodes.destroy();
updateIdleNodes(initialNodes);
inmem_hi = 0;
debugs(19, 9, this << " hi: " << inmem_hi);
}
Expand All @@ -72,8 +77,10 @@ mem_hdr::unlink(mem_node *aNode)
}

debugs(19, 8, this << " removing " << aNode);
const auto initialNodes = size();
nodes.remove (aNode, NodeCompare);
delete aNode;
updateIdleNodes(initialNodes);
return true;
}

Expand Down Expand Up @@ -320,18 +327,20 @@ mem_hdr::write (StoreIOBuffer const &writeBuffer)
char *currentSource = writeBuffer.data;
size_t len = writeBuffer.length;

const auto initialNodes = size();
while (len && (target = nodeToRecieve(currentOffset))) {
size_t wrote = writeAvailable(target, currentOffset, len, currentSource);
assert (wrote);
len -= wrote;
currentOffset += wrote;
currentSource += wrote;
}
updateIdleNodes(initialNodes);

return true;
}

mem_hdr::mem_hdr() : inmem_hi(0)
mem_hdr::mem_hdr() : inmem_hi(0), removableByReplPolicy(false)
{
debugs(19, 9, this << " hi: " << inmem_hi);
}
Expand Down Expand Up @@ -376,3 +385,40 @@ mem_hdr::getNodes() const
return nodes;
}

static void
UpdateIdleNodesCounter(const size_t oldSize, const size_t newSize)
{
if (newSize == oldSize)
return;
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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the IncreaseSum() address your concern?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marked this conversation as resolved.
Show resolved Hide resolved
debugs(19, 5, "Updated ReplPolicyIdleNodesCount from " << oldCount << " to " << mem_hdr::ReplPolicyIdleNodesCount);
}

void
mem_hdr::allowedToFreeWithReplPolicy(const bool allowed)
{
if (removableByReplPolicy == allowed)
return;
removableByReplPolicy = allowed;
const auto oldSize = removableByReplPolicy ? 0 : size();
const auto newSize = removableByReplPolicy ? size() : 0;
UpdateIdleNodesCounter(oldSize, newSize);
}

/// Adjusts the ReplPolicyIdleNodesCount counter by the difference
/// between the current size() and oldSize.
void
mem_hdr::updateIdleNodes(const size_t oldSize)
{
if (!removableByReplPolicy)
return;
UpdateIdleNodesCounter(oldSize, size());
}

10 changes: 9 additions & 1 deletion src/stmem.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ class mem_hdr
int64_t freeDataUpto (int64_t);
ssize_t copy (StoreIOBuffer const &) const;
bool hasContigousContentRange(Range<int64_t> const &range) const;
/* success or fail */
/// Saves the buffer into the internal storage.
/// Do not call directly - use StoreEntry::writeData() instead.
bool write (StoreIOBuffer const &);
rousskov marked this conversation as resolved.
Show resolved Hide resolved
void dump() const;
size_t size() const;
mem_node *getBlockContainingLocation (int64_t location) const;
void allowedToFreeWithReplPolicy(const bool allowed);
/* access the contained nodes - easier than punning
* as a container ourselves
*/
Expand All @@ -41,6 +43,10 @@ class mem_hdr

static Splay<mem_node *>::SPLAYCMP NodeCompare;

/// the total number of pages that allowed to be purged by
/// the associated replacement policy
static size_t ReplPolicyIdleNodesCount;

private:
void debugDump() const;
bool unlink(mem_node *aNode);
Expand All @@ -49,8 +55,10 @@ class mem_hdr
bool unionNotEmpty (StoreIOBuffer const &);
mem_node *nodeToRecieve(int64_t offset);
size_t writeAvailable(mem_node *aNode, int64_t location, size_t amount, char const *source);
void updateIdleNodes(const size_t oldSize);
int64_t inmem_hi;
Splay<mem_node *> nodes;
bool removableByReplPolicy; ///< whether nodes allowed to be purged by the associated replacement policy
};

#endif /* SQUID_STMEM_H */
Expand Down
32 changes: 29 additions & 3 deletions src/store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,8 @@ StoreEntry::hashDelete()
void
StoreEntry::lock(const char *context)
{
if (!lock_count && mem_obj && mem_obj->repl.data)
mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

++lock_count;
debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this);
}
Expand Down Expand Up @@ -450,6 +452,9 @@ StoreEntry::unlock(const char *context)
if (lock_count)
return (int) lock_count;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.


abandon(context);
return 0;
}
Expand Down Expand Up @@ -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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert(locked());

// XXX: caller uses content offset, but we also store headers
writeBuffer.offset += mem_obj->baseReply().hdr_sz;

debugs(20, 5, "storeWrite: writing " << writeBuffer.length << " bytes for '" << getMD5Text() << "'");
storeGetMemSpace(writeBuffer.length);
mem_obj->write(writeBuffer);
writeData(writeBuffer);

if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT) && !mem_obj->readAheadPolicyCanRead()) {
debugs(20, 3, "allow Store clients to get entry content after buffering too much for " << *this);
Expand All @@ -772,6 +777,16 @@ StoreEntry::write (StoreIOBuffer writeBuffer)
invokeHandlers();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed writeData() method.

auto &dataHdr = mem_obj->data_hdr;
assert (dataHdr.endOffset() || writeBuffer.offset == 0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert(dataHdr.write(writeBuffer));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this method survives, please respect NDEBUG builds, even though Squid has a few other bugs like this already.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed writeData() method.

}

/* Append incoming data from a primary server to an entry. */
void
StoreEntry::append(char const *buf, int len)
Expand Down Expand Up @@ -1513,13 +1528,17 @@ StoreEntry::setMemStatus(mem_status_t new_status)
} else {
mem_policy->Add(mem_policy, this, &mem_obj->repl);
debugs(20, 4, "inserted " << *this << " key: " << getMD5Text());
if (!locked())
mem_obj->data_hdr.allowedToFreeWithReplPolicy(true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied the first suggestion.

}

++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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

debugs(20, 4, "removed " << *this);
}
Expand Down Expand Up @@ -1557,7 +1576,7 @@ void
StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod)
{
if (!mem_obj)
mem_obj = new MemObject();
createMemObject();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo as no longer necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undone.

mem_obj->setUris(aUrl, aLogUrl, aMethod);
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove as unused?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

{
return mem_obj && mem_obj->repl.data;
}

/*
* return true if the entry is in a state where
* it can accept more data (ie with write() method)
Expand Down
4 changes: 4 additions & 0 deletions src/store/Controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,10 @@ Store::Controller::freeMemorySpace(const int bytesRequired)
if (memoryCacheHasSpaceFor(pagesRequired))
return;

// do not free anything if freeing everything freeable would not be enough
if (Less(mem_hdr::ReplPolicyIdleNodesCount, pagesRequired))
return;

// XXX: When store_pages_max is smaller than pagesRequired, we should not
// look for more space (but we do because we want to abandon idle entries?).

Expand Down
2 changes: 1 addition & 1 deletion src/store_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ store_client::readBody(const char *, ssize_t len)
* copyInto.offset includes headers, which is what mem cache needs
*/
if (copyInto.offset == entry->mem_obj->endOffset()) {
entry->mem_obj->write(StoreIOBuffer(len, copyInto.offset, copyInto.data));
entry->writeData(StoreIOBuffer(len, copyInto.offset, copyInto.data));
}
}
}
Expand Down
1 change: 0 additions & 1 deletion src/tests/stub_MemObject.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ int MemObject::mostBytesWanted(int, bool) const STUB_RETVAL(-1)
#if USE_DELAY_POOLS
DelayId MemObject::mostBytesAllowed() const STUB_RETVAL(DelayId())
#endif
void MemObject::write(const StoreIOBuffer &) STUB
int64_t MemObject::lowestMemReaderOffset() const STUB_RETVAL(0)
void MemObject::kickReads() STUB
int64_t MemObject::objectBytesOnDisk() const STUB_RETVAL(0)
Expand Down
1 change: 1 addition & 0 deletions src/tests/testStore.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ TestStore::testStats()
Store::Init(aStore);
CPPUNIT_ASSERT_EQUAL(false, aStore->statsCalled);
StoreEntry entry;
entry.lock("TestStore::testStats");
Store::Stats(&entry);
CPPUNIT_ASSERT_EQUAL(true, aStore->statsCalled);
Store::FreeMemory();
Expand Down
1 change: 1 addition & 0 deletions src/tests/testStoreController.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ TestStoreController::testStats()
{
Store::Init();
StoreEntry *logEntry = new StoreEntry;
logEntry->lock("TestStoreController::testStats");
logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod());
logEntry->store_status = STORE_PENDING;
TestSwapDirPointer aStore (new TestSwapDir);
Expand Down
1 change: 1 addition & 0 deletions src/tests/testStoreHashIndex.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ void
TestStoreHashIndex::testStats()
{
StoreEntry *logEntry = new StoreEntry;
logEntry->lock("TestStoreHashIndex::testStats");
logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod());
logEntry->store_status = STORE_PENDING;
Store::Init();
Expand Down