[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: proposed changes to product-queue module

>To: address@hidden
>From: Steve Emmerson <address@hidden>
>Subject: Re: 20040830: proposed changes to product-queue module
>Organization: Unidata
>Keywords: 200408302115.i7ULF28E010306

Steve,

> I could be wrong, but I think the follow changes need to be made to the
> product-queue module, pq.c:
> 
>     steve:~/ldm/package/src/pq: cvs diff -cw pq.c
>     Index: pq.c
>     ===================================================================
>     RCS file: /cvsroot/ldm/src/pq/pq.c,v
>     retrieving revision 3.78.8.2.2.7
>     diff -c -w -r3.78.8.2.2.7 pq.c
>     *** pq.c        29 Jul 2004 20:49:03 -0000      3.78.8.2.2.7
>     --- pq.c        30 Aug 2004 21:03:48 -0000
>     ***************
>     *** 657,663 ****
>             } /*else */
>             return (tqelem *) tpp;
>         case TV_EQ:
>     !         if(TV_CMP_EQ(tqp->tv, *key)) {
>                 return (tqelem *) tqp;
>             } /* else */
>             return NULL;
>     --- 657,663 ----
>             } /*else */
>             return (tqelem *) tpp;
>         case TV_EQ:
>     !         if(TV_CMP_EQ(tqp->tv, *key) && tqp->offset == coffset) {
>                 return (tqelem *) tqp;
>             } /* else */
>             return NULL;
>     ***************
>     *** 665,671 ****
>             if(q == TQ_NIL) {
>                 return NULL;
>             } /* else */
>     !         if(TV_CMP_EQ(tqp->tv, *key)) {
>                 q = fbp->fblks[tqp->fblk];
>                 tqp = &tq->tqep[q];
>                 if(q == TQ_NIL) {
>     --- 665,671 ----
>             if(q == TQ_NIL) {
>                 return NULL;
>             } /* else */
>     !         if(TV_CMP_EQ(tqp->tv, *key) && tqp->offset <= coffset) {
>                 q = fbp->fblks[tqp->fblk];
>                 tqp = &tq->tqep[q];
>                 if(q == TQ_NIL) {
> 
> The second change seems especially necessary; otherwise, it appears
> that an upstream LDM process will skip over an, otherwise, matching
> data-product that was inserted at the same time as a previous, matching
> data-product (at least, that's how it appears to me).
> 
> I'm less confident about the first change.
> 
> What do you think?

Off hand, I'd say the first change looks unnecessary, because whenever
the condition 

  TV_CMP_EQ(tqp->tv, *key) 

is true, I think at this point in the function

  tqp->offset == coffset

will also be true.  This is because this function is only invoked to
look for items that are known to have been inserted, so the previous
while loop guarantees that we can't have

  (TV_CMP_EQ(tqp->tv, *key) && tqp->offset < coffset)

hence we know 

  tqp->offset >= coffset

But if the product with the given coffset had been inserted, it will
be found, so we will in fact have

  tqp->offset == coffset

If you have actually encountered a case where  tqp->offset == coffset

  (TV_CMP_EQ(tqp->tv, *key) && tqp->offset > coffset

on leaving this while loop, I would be surprised.  You could ascertain
this empirically by inserting the assertion

assert(!TV_CMP_EQ(tqp->tv, *key) || (TV_CMP_EQ(tqp->tv, *key) &&
       tqp->offset == coffset);

after the (while(--k >= 0); statement, and run it using two ldms that
are close so you get lots of duplicate insertion times.  If you ever
see an assertion violation, I'll rethink this.  Or you can try to
convince me of some way this could happen.

I believe the same reasoning applies to the second proposed change,
which would also make this change unnecessary but benign, an extra
test for a clause that will always be true.  Again, an assertion
violation or cogent argument could convince me otherwise ...

--Russ