Merge ~bfrk/epics-base:scalar-get-optimization into ~epics-core/epics-base/+git/epics-base:7.0

[Ben Franksen]

rebased to current 7.0 and removed the dependency on the (already merged) remove-dbfl_type_rec

The idea here is to make the special optimization for one-element get requests without options available to all code that calls dbChannelGet. It started out as an attempt to disentangle this logic from dbDbGetValue (this goal is indeed achieved) and then moved on from that point.

[Ben Franksen (bfrk)]

I light of the bug I introduced with 3f13fff I will rebase this and the other branches I proposed for merging.

[mdavidsaver (mdavidsaver)]

Do you consider this MR ready? If so, please make an Approve review yourself. Or Needs Fixing if you have additional work planned.

wrt. changing the signature of dbGetField(), what is your plan for handling API compatibility with existing out of tree code? A quick search doesn't find as many usages as I was expecting, but there are some (eg. caPutLog)

https://github.com/search?q=org%3Aepics-modules+dbGetField&type=Code

I've a similar question about dbChannelGet*(), although this should have an easier answer.

In terms of the content, I'm not so enthusiastic about bulking up dbChannelGet() which has so far been a thin wrapper around dbGet(). Why can't this optimization be applied to dbGet()?

If this optimization to dbChannelGet() is judged worthwhile, some test coverage will be necessary.

[Ben Franksen (bfrk)]

I have re-submitted with an updated dependency. The type signature of dbGetField is unchanged. the change in the type signature for dbChannelGet merely makes the type of the last parameter more strongly typed. If some client passed something that is not a db_field_log* it would be an error.

Dirk has shown with a simple benchmark that the optimization is worthwhile for DB links, as Andrew had previously suspected. See the diuscussion starting at https://code.launchpad.net/~dirk.zimoch/epics-base/+git/epics-base/+merge/378968/comments/1000328

I deliberately did not move it to dbGet because dbGet with its multiple case distinctions is already quite a complicated beast. Entangling the optimization with that code would have been counter-productive. My goal was to disentangle it and put it in a single place, which is precisely why I chose dbChannelGet which, as you observed, was previously just a simple wrapper for dbGet.

review: Approve

[Ben Franksen (bfrk)]

Rebased. Also, the conditions under which the scalar get optimization applies now states them positively, moving the un-optimized cases into the else branch. This is clearer and more in line with the comments.

Unmerged commits

95da3b0... by Ben Franksen on 2020-03-25

move optimization for scalar requests from link to channel

The optimization is now available for all dbChannels, instead of only for DB
links, and also for requests involving filters. The code more clearly points
out where we optimize this special case and under what conditions.

diff --git a/modules/database/src/ioc/db/dbChannel.c b/modules/database/src/ioc/db/dbChannel.c
index c71d842..c1d996f 100644
--- a/modules/database/src/ioc/db/dbChannel.c
+++ b/modules/database/src/ioc/db/dbChannel.c
@@ -34,10 +34,12 @@
 #include "dbBase.h"
 #include "dbChannel.h"
 #include "dbCommon.h"
+#include "dbConvertFast.h"
 #include "dbEvent.h"
 #include "dbLock.h"
 #include "dbStaticLib.h"
 #include "link.h"
+#include "recGbl.h"
 #include "recSup.h"
 #include "special.h"
 #include "alarm.h"
@@ -630,14 +632,61 @@ long dbChannelOpen(dbChannel *chan)
 }
 
 /* Only use dbChannelGet() if the record is already locked. */
-long dbChannelGet(dbChannel *chan, short type, void *pbuffer,
-        long *options, long *nRequest, void *pfl)
+long dbChannelGet(dbChannel *chan, short dbrType, void *pbuffer,
+        long *options, long *nRequest, db_field_log *pfl)
 {
-    return dbGet(&chan->addr, type, pbuffer, options, nRequest, pfl);
+    dbAddr addr = chan->addr; /* structure copy */
+
+    if (pfl) {
+        addr.field_size = pfl->field_size;
+        addr.field_type = pfl->field_type;
+        addr.no_elements = pfl->no_elements;
+        addr.pfield = dbfl_pfield(pfl);
+    }
+
+    /*
+     * Try to optimize scalar requests by caching (just) the conversion
+     * routine. This is possible only if we have no options, the field and the
+     * request have exactly one element, we have no special processing to do,
+     * and the field type is not DBF_DEVICE or larger.
+     * Note that if the db_field_log has already copied the data, dbGet
+     * does not call get_array_info.
+     */
+    if (!(options && *options)
+        && addr.no_elements == 1
+        && (!nRequest || *nRequest == 1)
+        && (dbfl_has_copy(pfl) || addr.special != SPC_DBADDR)
+        && addr.special != SPC_ATTRIBUTE
+        && addr.field_type <= DBF_DEVICE)
+    {
+        if (chan->getCvt && chan->lastGetdbrType == dbrType) {
+            return chan->getCvt(addr.pfield, pbuffer, &addr);
+        } else {
+            unsigned short dbfType = addr.field_type;
+
+            if (INVALID_DB_REQ(dbrType)) {
+                char message[80];
+
+                sprintf(message, "dbChannelGet: Request type is %d\n", dbrType);
+                recGblDbaddrError(S_db_badDbrtype, &addr, message);
+                return S_db_badDbrtype;
+            }
+
+            chan->getCvt = dbFastGetConvertRoutine[dbfType][dbrType];
+            chan->lastGetdbrType = dbrType;
+            if (nRequest)
+                *nRequest = addr.no_elements;
+            return chan->getCvt(addr.pfield, pbuffer, &addr);
+        }
+    } else {
+        /* non-optimized case */
+        chan->getCvt = NULL;
+        return dbGet(&addr, dbrType, pbuffer, options, nRequest, pfl);
+    }
 }
 
 long dbChannelGetField(dbChannel *chan, short dbrType, void *pbuffer,
-        long *options, long *nRequest, void *pfl)
+        long *options, long *nRequest, db_field_log *pfl)
 {
     dbCommon *precord = chan->addr.precord;
     long status = 0;
diff --git a/modules/database/src/ioc/db/dbChannel.h b/modules/database/src/ioc/db/dbChannel.h
index ec86e9e..dc6240f 100644
--- a/modules/database/src/ioc/db/dbChannel.h
+++ b/modules/database/src/ioc/db/dbChannel.h
@@ -50,6 +50,8 @@ typedef struct evSubscrip {
 
 typedef struct chFilter chFilter;
 
+typedef long fastConvert(void *from, void *to, const dbAddr *paddr);
+
 /* A dbChannel points to a record field, and can have multiple filters */
 typedef struct dbChannel {
     const char *name;
@@ -60,6 +62,9 @@ typedef struct dbChannel {
     ELLLIST filters;          /* list of filters as created from JSON */
     ELLLIST pre_chain;        /* list of filters to be called pre-event-queue */
     ELLLIST post_chain;       /* list of filters to be called post-event-queue */
+    /* Support for optimizing scalar requests */
+    fastConvert *getCvt;      /* fast get conversion function */
+    short lastGetdbrType;     /* last dbChannelGet dbrType */
 } dbChannel;
 
 /* Prototype for the channel event function that is called in filter stacks
@@ -208,9 +213,9 @@ DBCORE_API extern unsigned short dbDBRnewToDBRold[];
 
 
 DBCORE_API long dbChannelGet(dbChannel *chan, short type,
-        void *pbuffer, long *options, long *nRequest, void *pfl);
+        void *pbuffer, long *options, long *nRequest, db_field_log *pfl);
 DBCORE_API long dbChannelGetField(dbChannel *chan, short type,
-        void *pbuffer, long *options, long *nRequest, void *pfl);
+        void *pbuffer, long *options, long *nRequest, db_field_log *pfl);
 DBCORE_API long dbChannelPut(dbChannel *chan, short type,
         const void *pbuffer, long nRequest);
 DBCORE_API long dbChannelPutField(dbChannel *chan, short type,
diff --git a/modules/database/src/ioc/db/dbDbLink.c b/modules/database/src/ioc/db/dbDbLink.c
index b281314..399d36d 100644
--- a/modules/database/src/ioc/db/dbDbLink.c
+++ b/modules/database/src/ioc/db/dbDbLink.c
@@ -58,7 +58,6 @@
 #include "dbBase.h"
 #include "dbBkpt.h"
 #include "dbCommonPvt.h"
-#include "dbConvertFast.h"
 #include "dbConvert.h"
 #include "db_field_log.h"
 #include "db_access_routines.h"
@@ -135,9 +134,7 @@ static void dbDbRemoveLink(struct dbLocker *locker, struct link *plink)
     /* locker is NULL when an isolated IOC is closing its links */
     if (locker) {
         plink->value.pv_link.pvt = 0;
-        plink->value.pv_link.getCvt = 0;
         plink->value.pv_link.pvlMask = 0;
-        plink->value.pv_link.lastGetdbrType = 0;
         ellDelete(&precord->bklnk, &plink->value.pv_link.backlinknode);
         dbLockSetSplit(locker, plink->precord, precord);
     }
@@ -167,7 +164,6 @@ static long dbDbGetValue(struct link *plink, short dbrType, void *pbuffer,
 {
     struct pv_link *ppv_link = &plink->value.pv_link;
     dbChannel *chan = linkChannel(plink);
-    DBADDR *paddr = &chan->addr;
     dbCommon *precord = plink->precord;
     db_field_log *pfl = NULL;
     long status;
@@ -179,49 +175,19 @@ static long dbDbGetValue(struct link *plink, short dbrType, void *pbuffer,
             return status;
     }
 
-    if (ppv_link->getCvt && ppv_link->lastGetdbrType == dbrType)
-    {
-        /* shortcut: scalar with known conversion, no filter */
-        status = ppv_link->getCvt(dbChannelField(chan), pbuffer, paddr);
-    }
-    else if (dbChannelFinalElements(chan) == 1 && (!pnRequest || *pnRequest == 1)
-                && dbChannelSpecial(chan) != SPC_DBADDR
-                && dbChannelSpecial(chan) != SPC_ATTRIBUTE
-                && ellCount(&chan->filters) == 0)
-    {
-        /* simple scalar: set up shortcut */
-        unsigned short dbfType = dbChannelFinalFieldType(chan);
-
-        if (dbrType < 0 || dbrType > DBR_ENUM || dbfType > DBF_DEVICE)
-            return S_db_badDbrtype;
-
-        ppv_link->getCvt = dbFastGetConvertRoutine[dbfType][dbrType];
-        ppv_link->lastGetdbrType = dbrType;
-        status = ppv_link->getCvt(dbChannelField(chan), pbuffer, paddr);
-    }
-    else
-    {
-        /* filter, array, or special */
-        ppv_link->getCvt = NULL;
-
-        if (ellCount(&chan->filters)) {
-            /* If filters are involved in a read, create field log and run filters */
-            pfl = db_create_read_log(chan);
-            if (!pfl)
-                return S_db_noMemory;
-
-            pfl = dbChannelRunPreChain(chan, pfl);
-            pfl = dbChannelRunPostChain(chan, pfl);
-        }
-
-        status = dbChannelGet(chan, dbrType, pbuffer, NULL, pnRequest, pfl);
-
-        if (pfl)
-            db_delete_field_log(pfl);
-
-        if (status)
-            return status;
+    /* If filters are involved in a read, create field log and run filters */
+    if (ellCount(&chan->filters)) {
+        pfl = db_create_read_log(chan);
+        if (!pfl)
+            return S_db_noMemory;
+        pfl = dbChannelRunPreChain(chan, pfl);
+        pfl = dbChannelRunPostChain(chan, pfl);
     }
+    status = dbChannelGet(chan, dbrType, pbuffer, NULL, pnRequest, pfl);
+    if (pfl)
+        db_delete_field_log(pfl);
+    if (status)
+        return status;
 
     if (!status && precord != dbChannelRecord(chan))
         recGblInheritSevr(plink->value.pv_link.pvlMask & pvlOptMsMode,
diff --git a/modules/database/src/ioc/dbStatic/link.h b/modules/database/src/ioc/dbStatic/link.h
index c3c0045..d212395 100644
--- a/modules/database/src/ioc/dbStatic/link.h
+++ b/modules/database/src/ioc/dbStatic/link.h
@@ -78,15 +78,12 @@ struct macro_link {
 };
 
 struct dbCommon;
-typedef long (*LINKCVT)();
 
 struct pv_link {
     ELLNODE     backlinknode;
     char        *pvname;        /* pvname link points to */
     void        *pvt;           /* CA or DB private */
-    LINKCVT     getCvt;         /* input conversion function */
     short       pvlMask;        /* Options mask */
-    short       lastGetdbrType; /* last dbrType for DB or CA get */
 };
 
 struct jlink;