Re: [Bug 1359975] pmchart run-away mem leak replaying multi-archive when rewinding
"Dave Brolley" <[email protected]>
| Newsgroups | gmane.comp.sysutils.pcp |
|---|---|
| Message-ID | <[email protected]> |
Please find attached 3 proposed patches: *__pmTimevalCmp.patch.txt*: __pmtimevalCmp() takes two arguments of type 'struct timeval' where we want to compare two arguments of type __pmTimeval. Rather than introduce the inconvenience and inefficiency of converting before comparing, I created a similar new function, __pmTimevalCmp() to compare them directly. *ordered.patch.txt*: Uses memcmp within sameindom() to compare the instances, assuming that the instances within instance domains will always be ordered in the same way. *unordered.patch.txt*: Alternate implementation of sameindom() to be used if we can not assume that the instances within instance domains will always be ordered in the same way. Dave On 12/13/2016 12:05 PM, Dave Brolley wrote: > Thanks -- I've picked up the style/typo patch, including the final > typo for __pmLogIndom. Working on the rest. > > Hopefully Ken knows whether we can rely on consistent ordering of > instances and instance names. If not, the current implementation would > miss some duplicates. FWIW, the massive build up of duplicates > displayed by Mark's test case was tamed by the current implementation, > suggesting that most, if not all, of the duplicates in those archives > were sorted in the same way. > > Dave > > On 12/12/2016 10:39 PM, Nathan Scott wrote: >> Hi Dave, >> >> ----- Original Message ----- >>> https://bugzilla.redhat.com/show_bug.cgi?id=1359975 >>> >>> --- Comment #17 from Dave Brolley <[email protected]> --- >>> Additional proposed commit (on top of the previous one): >>> >>> In the tree/branch: git://git.pcp.io/brolley/pcp rhbz1359975 >>> >>> commit 82e02ffea204d26e7617a4bcf4be0b9326c64457 >>> Author: Dave Brolley <[email protected]> >>> Date: Mon Dec 12 14:39:09 2016 -0500 >>> >>> RHBZ 1359975: pmchart run-away mem leak replaying multi-archive >>> when >>> rewinding >>> >>> Refinement. Exposed by qa regressions. Ensure that instance >>> domains are >>> in the correct order in the hash chain: >>> >>> - Primary sort by timestamp (descending) >>> - Secondary sort: latest added at the head of each time slot. This >>> includes moving duplicates to the head of their time slot when >>> detected. >>> >>> Before multi-archive contexts and duplicate-filtering, this >>> happened >>> automatically. >> Couple things - the new sameindom() routine seems to assuming the >> instance >> IDs and names are sorted in some way (i.e. direct array comparison >> can be >> done, rather than iterating through the 2nd array for each element in >> the >> 1st - is that the case?) Not sure. >> >> Other, small thing - it would be more efficient to add __pmtimevalCmp >> into >> libpcp now I think (its in libpcp_gui for hysterical raisins IIRC), >> so it's >> available for use here instead of using floating point math & >> comparisons >> as is being done for the timestamps now. >> >> Attached patch has some minor pcp-style and typo cleanups. >> >> cheers. >> >> -- >> Nathan > > > >
__pmTimeCmp.patch.txt
(text/plain, 3.1 KB)
diff --git a/src/include/pcp/impl.h b/src/include/pcp/impl.h
index df65ba6..fbe2a8d 100644
--- a/src/include/pcp/impl.h
+++ b/src/include/pcp/impl.h
@@ -1170,6 +1170,7 @@ PCP_CALL extern int __pmParseTime(const char *, struct timeval *, struct timeval
struct timeval *, char **);
/* manipulate internal timestamps */
+PCP_CALL extern int __pmTimevalCmp(const __pmTimeval *, const __pmTimeval *);
PCP_CALL extern double __pmTimevalSub(const __pmTimeval *, const __pmTimeval *);
/* 32-bit file checksum */
diff --git a/src/libpcp/src/logmeta.c b/src/libpcp/src/logmeta.c
index c7463ed..2005c92 100644
--- a/src/libpcp/src/logmeta.c
+++ b/src/libpcp/src/logmeta.c
@@ -86,7 +86,7 @@ addindom(__pmLogCtl *lcp, pmInDom indom, const __pmTimeval *tp, int numinst,
__pmLogInDom *idp, *idp_prev;
__pmLogInDom *idp_cached, *idp_time;
__pmHashNode *hp;
- double timediff;
+ int timecmp;
int sts;
PM_FAULT_POINT("libpcp/" __FILE__ ":1", PM_FAULT_ALLOC);
@@ -132,13 +132,13 @@ PM_FAULT_POINT("libpcp/" __FILE__ ":1", PM_FAULT_ALLOC);
*/
idp_prev = NULL;
for (idp_cached = (__pmLogInDom *)hp->data; idp_cached; idp_cached = idp_cached->next) {
- timediff = __pmTimevalSub(&idp_cached->stamp, &idp->stamp);
+ timecmp = __pmTimevalCmp(&idp_cached->stamp, &idp->stamp);
/*
* If the time of the current cached item is before our time,
* then insert here.
*/
- if (timediff < 0)
+ if (timecmp < 0)
break;
/*
@@ -147,7 +147,7 @@ PM_FAULT_POINT("libpcp/" __FILE__ ":1", PM_FAULT_ALLOC);
* to the head of this time slot. Otherwise insert this new item
* at the head of the time slot.
*/
- if (timediff == 0) {
+ if (timecmp == 0) {
sts = 0;
idp_time = idp_prev; /* just before this time slot */
do {
@@ -161,8 +161,8 @@ PM_FAULT_POINT("libpcp/" __FILE__ ":1", PM_FAULT_ALLOC);
idp_cached = idp_cached->next;
if (idp_cached == NULL)
break;
- timediff = __pmTimevalSub(&idp_cached->stamp, &idp->stamp);
- } while (timediff == 0);
+ timecmp = __pmTimevalCmp(&idp_cached->stamp, &idp->stamp);
+ } while (timecmp == 0);
if (sts == 1) {
/*
@@ -674,7 +674,7 @@ searchindom(__pmLogCtl *lcp, pmInDom indom, __pmTimeval *tp)
/*
* need first one at or earlier than the requested time
*/
- if (__pmTimevalSub(&idp->stamp, tp) <= 0)
+ if (__pmTimevalCmp(&idp->stamp, tp) <= 0)
break;
#ifdef PCP_DEBUG
if (pmDebug & DBG_TRACE_LOGMETA) {
diff --git a/src/libpcp/src/util.c b/src/libpcp/src/util.c
index 61a83fb..f091626 100644
--- a/src/libpcp/src/util.c
+++ b/src/libpcp/src/util.c
@@ -1056,6 +1056,20 @@ __pmGetTimespec(struct timespec *ts)
}
/*
+ * a : b for __pmTimeval ... <0 for a<b, ==0 for a==b, >0 for a>b
+ */
+int
+__pmTimevalCmp(const __pmTimeval *a, const __pmTimeval *b)
+{
+ int res = (int)(a->tv_sec - b->tv_sec);
+
+ if (res == 0)
+ res = (int)(a->tv_usec - b->tv_usec);
+
+ return res;
+}
+
+/*
* Difference for two of the internal timestamps ...
* Same as __pmtimevalSub() in tv.c, just with __pmTimeval args
* rather than struct timeval args.
ordered.patch.txt
(text/plain, 660 B)
diff --git a/src/libpcp/src/logmeta.c b/src/libpcp/src/logmeta.c
index 2005c92..f64a5f3 100644
--- a/src/libpcp/src/logmeta.c
+++ b/src/libpcp/src/logmeta.c
@@ -46,9 +46,11 @@ sameindom(const __pmLogInDom *idp1, const __pmLogInDom *idp2)
if (idp1->numinst != idp2->numinst)
return 0; /* different */
+ if (memcmp(idp1->instlist, idp2->instlist,
+ idp1->numinst * sizeof(*idp1->instlist)) != 0)
+ return 0; /* different */
+
for (i = 0; i < idp1->numinst; ++i) {
- if (idp1->instlist[i] != idp2->instlist[i])
- return 0; /* different */
if (strcmp(idp1->namelist[i], idp2->namelist[i]) != 0)
return 0; /* different */
}
unordered.patch.txt
(text/plain, 1.2 KB)
diff --git a/src/libpcp/src/logmeta.c b/src/libpcp/src/logmeta.c
index 2005c92..3d0cd05 100644
--- a/src/libpcp/src/logmeta.c
+++ b/src/libpcp/src/logmeta.c
@@ -41,16 +41,32 @@ StrTimeval(const __pmTimeval *tp)
static int
sameindom(const __pmLogInDom *idp1, const __pmLogInDom *idp2)
{
- int i;
+ int i, j;
if (idp1->numinst != idp2->numinst)
return 0; /* different */
+ /*
+ * Make sure that the instances and their names are the same.
+ * We can't assume that the instances are always in the same order,
+ * but we do assume that each instance occurs only once.
+ */
for (i = 0; i < idp1->numinst; ++i) {
- if (idp1->instlist[i] != idp2->instlist[i])
- return 0; /* different */
- if (strcmp(idp1->namelist[i], idp2->namelist[i]) != 0)
+ for (j = 0; j < idp2->numinst; ++j) {
+ if (idp1->instlist[i] == idp2->instlist[j]) {
+ /*
+ * We found the same instance. Make sure that the names are
+ * the same.
+ */
+ if (strcmp(idp1->namelist[i], idp2->namelist[j]) != 0)
+ return 0; /* different */
+ break;
+ }
+ }
+ if (j >= idp2->numinst) {
+ /* The current idp1 instance was not found in idp2. */
return 0; /* different */
+ }
}
return 1; /* duplicate */