From 69565793eef89d0137a69e2d923f14adc051b173 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 16 Jul 2018 12:47:52 +0000 Subject: [PATCH] Remove NullStoreEntry (#241) A nil pointer is the proper way to indicate a missing heap-allocated object in C++. Removing NullStoreEntry simplifies and optimizes code. This removal also brings us one step closer to removing all virtual methods from StoreEntry, further optimizing code and even saving 8 bytes per non-shared memory cache entry on most platforms. Also un-virtualized a few StoreEntry-only methods to optimize their callers. --- src/Store.h | 64 ++++++++++------------------------------ src/StoreClient.h | 2 +- src/client_side_reply.cc | 34 ++++++++------------- src/htcp.cc | 8 ++--- src/icp_v2.cc | 7 ++--- src/icp_v3.cc | 4 +-- src/mime.cc | 2 +- src/store.cc | 52 ++------------------------------ src/tests/stub_store.cc | 6 ---- src/tests/testStore.cc | 3 +- src/urn.cc | 4 +-- 11 files changed, 44 insertions(+), 142 deletions(-) diff --git a/src/Store.h b/src/Store.h index bf20ae7f50..c5a31c4ca1 100644 --- a/src/Store.h +++ b/src/Store.h @@ -45,36 +45,36 @@ public: static DeferredRead::DeferrableRead DeferReader; bool checkDeferRead(int fd) const; - virtual const char *getMD5Text() const; + const char *getMD5Text() const; StoreEntry(); virtual ~StoreEntry(); - virtual HttpReply const *getReply() const; - virtual void write (StoreIOBuffer); + HttpReply const *getReply() const; + void write(StoreIOBuffer); - /** Check if the Store entry is emtpty + /** Check if the Store entry is empty * \retval true Store contains 0 bytes of data. * \retval false Store contains 1 or more bytes of data. * \retval false Store contains negative content !!!!!! */ - virtual bool isEmpty() const { + bool isEmpty() const { assert (mem_obj); return mem_obj->endOffset() == 0; } - virtual bool isAccepting() const; - virtual size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; + bool isAccepting() const; + size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it void lengthWentBad(const char *reason); - virtual void complete(); - virtual store_client_t storeClientType() const; - virtual char const *getSerialisedMetaData(); + void complete(); + store_client_t storeClientType() const; + char const *getSerialisedMetaData(); /// Store a prepared error response. MemObject locks the reply object. void storeErrorResponse(HttpReply *reply); void replaceHttpReply(HttpReply *, bool andStartWriting = true); void startWriting(); ///< pack and write reply headers and, maybe, body /// whether we may start writing to disk (now or in the future) - virtual bool mayStartSwapOut(); - virtual void trimMemory(const bool preserveSwappable); + bool mayStartSwapOut(); + void trimMemory(const bool preserveSwappable); // called when a decision to cache in memory has been made void memOutDecision(const bool willCacheInRam); @@ -227,16 +227,14 @@ public: static void getPublicByRequest(StoreClient * aClient, HttpRequest * request); static void getPublic(StoreClient * aClient, const char *uri, const HttpRequestMethod& method); - virtual bool isNull() const { return false; } // TODO: Replace with nullptr. - void *operator new(size_t byteCount); void operator delete(void *address); #if USE_SQUID_ESI ESIElement::Pointer cachedESITree; #endif - virtual int64_t objectLen() const; - virtual int64_t contentLen() const; + int64_t objectLen() const; + int64_t contentLen() const; /// claim shared ownership of this entry (for use in a given context) /// matching lock() and unlock() contexts eases leak triage but is optional @@ -256,8 +254,7 @@ public: /// Removes all unlocked (and marks for eventual removal all locked) Store /// entries, including attached and unattached entries that have our key. /// Also destroys us if we are unlocked or makes us private otherwise. - /// TODO: remove virtual. - virtual void release(const bool shareable = false); + void release(const bool shareable = false); /// One of the three methods to get rid of an unlocked StoreEntry object. /// May destroy this object if it is unlocked; does nothing otherwise. @@ -322,37 +319,6 @@ private: std::ostream &operator <<(std::ostream &os, const StoreEntry &e); -/// \ingroup StoreAPI -class NullStoreEntry:public StoreEntry -{ - -public: - static NullStoreEntry *getInstance(); - - const char *getMD5Text() const; - HttpReply const *getReply() const { return NULL; } - void write (StoreIOBuffer) {} - - bool isEmpty () const {return true;} - - /* StoreEntry API */ - virtual bool isNull() const { return true; } - virtual size_t bytesWanted(Range const aRange, bool) const { return aRange.end; } - - void operator delete(void *address); - void complete() {} - -private: - store_client_t storeClientType() const {return STORE_MEM_CLIENT;} - - char const *getSerialisedMetaData(); - virtual bool mayStartSwapOut() { return false; } - - void trimMemory(const bool) {} - - static NullStoreEntry _instance; -}; - /// \ingroup StoreAPI typedef void (*STOREGETCLIENT) (StoreEntry *, void *cbdata); diff --git a/src/StoreClient.h b/src/StoreClient.h index 2d4f92953c..e9661bc0dc 100644 --- a/src/StoreClient.h +++ b/src/StoreClient.h @@ -29,7 +29,7 @@ public: // TODO: Remove? Probably added to make lookups asynchronous, but they are // still blocking. A lot more is needed to support async callbacks. /// Handle a StoreEntry::getPublic*() result. - /// An isNull() entry indicates a cache miss. + /// A nil entry indicates a cache miss. virtual void created(StoreEntry *) = 0; /// \return LogTags (if the class logs transactions) or nil (otherwise) diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 1fb456067c..1366bd0b59 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -959,7 +959,7 @@ clientReplyContext::loggingTags() void clientReplyContext::purgeFoundGet(StoreEntry *newEntry) { - if (newEntry->isNull()) { + if (!newEntry) { lookingforstore = 2; StoreEntry::getPublicByRequestMethod(this, http->request, Http::METHOD_HEAD); } else @@ -969,7 +969,7 @@ clientReplyContext::purgeFoundGet(StoreEntry *newEntry) void clientReplyContext::purgeFoundHead(StoreEntry *newEntry) { - if (newEntry->isNull()) + if (!newEntry) purgeDoMissPurge(); else purgeFoundObject (newEntry); @@ -978,7 +978,7 @@ clientReplyContext::purgeFoundHead(StoreEntry *newEntry) void clientReplyContext::purgeFoundObject(StoreEntry *entry) { - assert (entry && !entry->isNull()); + assert (entry); if (EBIT_TEST(entry->flags, ENTRY_SPECIAL)) { http->logType.update(LOG_TCP_DENIED); @@ -1051,11 +1051,9 @@ clientReplyContext::purgeDoMissPurge() void clientReplyContext::purgeDoPurgeGet(StoreEntry *newEntry) { - assert (newEntry); - /* Move to new() when that is created */ - purgeStatus = Http::scNotFound; - - if (!newEntry->isNull()) { + if (newEntry) { + /* Move to new() when that is created */ + purgeStatus = Http::scNotFound; /* Release the cached URI */ debugs(88, 4, "clientPurgeRequest: GET '" << newEntry->url() << "'" ); #if USE_HTCP @@ -1072,7 +1070,7 @@ clientReplyContext::purgeDoPurgeGet(StoreEntry *newEntry) void clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) { - if (newEntry && !newEntry->isNull()) { + if (newEntry) { debugs(88, 4, "HEAD " << newEntry->url()); #if USE_HTCP neighborsHtcpClear(newEntry, NULL, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE); @@ -1686,7 +1684,7 @@ clientReplyContext::identifyStoreObject() lookingforstore = 5; StoreEntry::getPublicByRequest (this, r); } else { - identifyFoundObject (NullStoreEntry::getInstance()); + identifyFoundObject(nullptr); } } @@ -1697,17 +1695,9 @@ clientReplyContext::identifyStoreObject() void clientReplyContext::identifyFoundObject(StoreEntry *newEntry) { - StoreEntry *e = newEntry; HttpRequest *r = http->request; - - /** \li If the entry received isNull() then we ignore it. */ - if (e->isNull()) { - http->storeEntry(NULL); - } else { - http->storeEntry(e); - } - - e = http->storeEntry(); + http->storeEntry(newEntry); + const auto e = http->storeEntry(); /* Release IP-cache entries on reload */ /** \li If the request has no-cache flag set or some no_cache HACK in operation we @@ -1717,10 +1707,10 @@ clientReplyContext::identifyFoundObject(StoreEntry *newEntry) ipcacheInvalidateNegative(r->url.host()); #if USE_CACHE_DIGESTS - lookup_type = http->storeEntry() ? "HIT" : "MISS"; + lookup_type = e ? "HIT" : "MISS"; #endif - if (NULL == http->storeEntry()) { + if (!e) { /** \li If no StoreEntry object is current assume this object isn't in the cache set MISS*/ debugs(85, 3, "StoreEntry is NULL - MISS"); http->logType.update(LOG_TCP_MISS); diff --git a/src/htcp.cc b/src/htcp.cc index 473fab17aa..8c3fa71026 100644 --- a/src/htcp.cc +++ b/src/htcp.cc @@ -919,14 +919,14 @@ htcpSpecifier::checkHit() if (!checkHitRequest) { debugs(31, 3, "htcpCheckHit: NO; failed to parse URL"); - checkedHit(NullStoreEntry::getInstance()); + checkedHit(nullptr); return; } if (!checkHitRequest->header.parse(req_hdrs, reqHdrsSz)) { debugs(31, 3, "htcpCheckHit: NO; failed to parse request headers"); checkHitRequest = nullptr; - checkedHit(NullStoreEntry::getInstance()); + checkedHit(nullptr); return; } @@ -938,7 +938,7 @@ htcpSpecifier::created(StoreEntry *e) { StoreEntry *hit = nullptr; - if (!e || e->isNull()) { + if (!e) { debugs(31, 3, "htcpCheckHit: NO; public object not found"); } else if (!e->validToSend()) { debugs(31, 3, "htcpCheckHit: NO; entry not valid to send" ); @@ -954,7 +954,7 @@ htcpSpecifier::created(StoreEntry *e) checkedHit(hit); // TODO: StoreClients must either store/lock or abandon found entries. - //if (!e->isNull()) + //if (e) // e->abandon(); } diff --git a/src/icp_v2.cc b/src/icp_v2.cc index 756df4eeef..3a56b690ab 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -156,9 +156,6 @@ ICPState::~ICPState() bool ICPState::confirmAndPrepHit(const StoreEntry &e) { - if (e.isNull()) - return false; - if (!e.validToSend()) return false; @@ -217,7 +214,7 @@ ICP2State::created(StoreEntry *e) debugs(12, 5, "icpHandleIcpV2: OPCODE " << icp_opcode_str[header.opcode]); icp_opcode codeToSend; - if (confirmAndPrepHit(*e)) { + if (e && confirmAndPrepHit(*e)) { codeToSend = ICP_HIT; } else { #if USE_ICMP @@ -238,7 +235,7 @@ ICP2State::created(StoreEntry *e) icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from, al); // TODO: StoreClients must either store/lock or abandon found entries. - //if (!e->isNull()) + //if (e) // e->abandon(); delete this; diff --git a/src/icp_v3.cc b/src/icp_v3.cc index 4541cf89a4..89b2f25ee1 100644 --- a/src/icp_v3.cc +++ b/src/icp_v3.cc @@ -66,7 +66,7 @@ ICP3State::created(StoreEntry *e) debugs(12, 5, "icpHandleIcpV3: OPCODE " << icp_opcode_str[header.opcode]); icp_opcode codeToSend; - if (confirmAndPrepHit(*e)) { + if (e && confirmAndPrepHit(*e)) { codeToSend = ICP_HIT; } else if (icpGetCommonOpcode() == ICP_ERR) codeToSend = ICP_MISS; @@ -76,7 +76,7 @@ ICP3State::created(StoreEntry *e) icpCreateAndSend(codeToSend, 0, url, header.reqnum, 0, fd, from, al); // TODO: StoreClients must either store/lock or abandon found entries. - //if (!e->isNull()) + //if (e) // e->abandon(); delete this; diff --git a/src/mime.cc b/src/mime.cc index efdbfd06e1..98460b35d5 100644 --- a/src/mime.cc +++ b/src/mime.cc @@ -363,7 +363,7 @@ void MimeIcon::created(StoreEntry *newEntry) { /* if the icon is already in the store, do nothing */ - if (!newEntry->isNull()) + if (newEntry) return; // XXX: if a 204 is cached due to earlier load 'failure' we should try to reload. diff --git a/src/store.cc b/src/store.cc index 6d5e355451..49013e3a0c 100644 --- a/src/store.cc +++ b/src/store.cc @@ -393,9 +393,6 @@ destroyStoreEntry(void *data) StoreEntry *e = static_cast(static_cast(data)); assert(e != NULL); - if (e == NullStoreEntry::getInstance()) - return; - // Store::Root() is FATALly missing during shutdown if (e->hasDisk() && !shutting_down) e->disk().disconnect(*e); @@ -498,36 +495,21 @@ void StoreEntry::getPublicByRequestMethod (StoreClient *aClient, HttpRequest * request, const HttpRequestMethod& method) { assert (aClient); - StoreEntry *result = storeGetPublicByRequestMethod( request, method); - - if (!result) - aClient->created (NullStoreEntry::getInstance()); - else - aClient->created (result); + aClient->created(storeGetPublicByRequestMethod(request, method)); } void StoreEntry::getPublicByRequest (StoreClient *aClient, HttpRequest * request) { assert (aClient); - StoreEntry *result = storeGetPublicByRequest (request); - - if (!result) - result = NullStoreEntry::getInstance(); - - aClient->created (result); + aClient->created(storeGetPublicByRequest(request)); } void StoreEntry::getPublic (StoreClient *aClient, const char *uri, const HttpRequestMethod& method) { assert (aClient); - StoreEntry *result = storeGetPublic (uri, method); - - if (!result) - result = NullStoreEntry::getInstance(); - - aClient->created (result); + aClient->created(storeGetPublic(uri, method)); } StoreEntry * @@ -2168,34 +2150,6 @@ std::ostream &operator <<(std::ostream &os, const StoreEntry &e) return os << '/' << &e << '*' << e.locks(); } -/* NullStoreEntry */ - -NullStoreEntry NullStoreEntry::_instance; - -NullStoreEntry * -NullStoreEntry::getInstance() -{ - return &_instance; -} - -char const * -NullStoreEntry::getMD5Text() const -{ - return "N/A"; -} - -void -NullStoreEntry::operator delete(void*) -{ - fatal ("Attempt to delete NullStoreEntry\n"); -} - -char const * -NullStoreEntry::getSerialisedMetaData() -{ - return NULL; -} - void Store::EntryGuard::onException() noexcept { diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index 2228187285..7c81acf79d 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -97,12 +97,6 @@ void StoreEntry::append(char const *, int) STUB void StoreEntry::vappendf(const char *, va_list) STUB void StoreEntry::setCollapsingRequirement(const bool required) STUB -NullStoreEntry *NullStoreEntry::getInstance() STUB_RETVAL(NULL) -const char *NullStoreEntry::getMD5Text() const STUB_RETVAL(NULL) -void NullStoreEntry::operator delete(void *address) STUB -// private virtual. Why is this linked from outside? -const char *NullStoreEntry::getSerialisedMetaData() STUB_RETVAL(NULL) - Store::Controller &Store::Root() STUB_RETREF(Store::Controller) void Store::Init(Store::Controller *root) STUB void Store::FreeMemory() STUB diff --git a/src/tests/testStore.cc b/src/tests/testStore.cc index b29a7f8508..5de9c700ee 100644 --- a/src/tests/testStore.cc +++ b/src/tests/testStore.cc @@ -108,7 +108,8 @@ testStore::testStats() TestStore *aStore(new TestStore); Store::Init(aStore); CPPUNIT_ASSERT_EQUAL(false, aStore->statsCalled); - Store::Stats(NullStoreEntry::getInstance()); + StoreEntry entry; + Store::Stats(&entry); CPPUNIT_ASSERT_EQUAL(true, aStore->statsCalled); Store::FreeMemory(); } diff --git a/src/urn.cc b/src/urn.cc index 976d37f36b..37e0d4f3f2 100644 --- a/src/urn.cc +++ b/src/urn.cc @@ -199,12 +199,12 @@ UrnState::fillChecklist(ACLFilledChecklist &checklist) const void UrnState::created(StoreEntry *e) { - if (e->isNull() || (e->hittingRequiresCollapsing() && !startCollapsingOn(*e, false))) { + if (!e || (e->hittingRequiresCollapsing() && !startCollapsingOn(*e, false))) { urlres_e = storeCreateEntry(urlres, urlres, RequestFlags(), Http::METHOD_GET); sc = storeClientListAdd(urlres_e, this); FwdState::Start(Comm::ConnectionPointer(), urlres_e, urlres_r.getRaw(), ale); // TODO: StoreClients must either store/lock or abandon found entries. - //if (!e->isNull()) + //if (e) // e->abandon(); } else { urlres_e = e; -- 2.39.5