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.

diff --git a/src/Store.h b/src/Store.h
index bf20ae7f5061ae5bf28680f2af9361b81c509362..c5a31c4ca1101b9925e231aac5ad66100db3a747 100644 (file)
--- 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<size_t> const aRange, bool ignoreDelayPool = false) const;
+    bool isAccepting() const;
+    size_t bytesWanted(Range<size_t> 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<size_t> 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 2d4f92953cecd0335f244293c340ff03ee970702..e9661bc0dcad273959cbc4bb531d1a82eec7840a 100644 (file)
--- 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 1fb456067c2c434f94ecb6d68c04d364a3f64b0b..1366bd0b59fe0c9dc36d9acaa9b3c5b19fad59c3 100644 (file)
--- 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 473fab17aaf90817bf3ee65324802585fff6b54e..8c3fa7102629b2270be39d09bf39f1931a77bdcb 100644 (file)
--- 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 756df4eeef4338a92395578997324e2c0f6c3980..3a56b690abd171f2d376b197a53d61b559a96c53 100644 (file)
--- 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 4541cf89a45ff1d295c663cd2de3468a41917581..89b2f25ee10b254c1b32259d4ee5a2d01291e335 100644 (file)
--- 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 efdbfd06e17c08ad4e8a58fc81fe704733582993..98460b35d59f79c32f4eb7e80f945c98b85f5bdc 100644 (file)
--- 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 6d5e355451951e917ef208745eae95bf59c4dd25..49013e3a0c124fa51ba96412378b14dc76cad423 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -393,9 +393,6 @@ destroyStoreEntry(void *data)
     StoreEntry *e = static_cast<StoreEntry *>(static_cast<hash_link *>(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 22281872850170b0a050514567d7331285a3b6f8..7c81acf79d81646d603d545387e4e9f351b38a32 100644 (file)
--- 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 b29a7f85084816c74e5dafc1599107c4768167af..5de9c700ee62c11e47f3ae38ddc1b368beea029d 100644 (file)
--- 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 976d37f36ba1ccf8e381eb24041ab7f8bebe3664..37e0d4f3f2f6c8a51a28c262fc211eae9be4b887 100644 (file)
--- 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;