FWD » Core Development » Database

While investigating #10247, I found a scenario where an entry is not being evicted from FFcache at the invalidation stage. This happens when there are 2 queries for the same record. Only the entry associated to a query is deleted. This is a testcase to reproduce the issue:

def temp-table table3 field f1 as int field f2 as logical index idx_1 is primary unique f1.
create table3.
table3.f1 = 1.
table3.f2 = false.

find first table3 where f1 = 1 and not  table3.f2 no-error.
find first table3 where f1 < 10 and not  table3.f2 no-error.

assign
table3.f2 = true
.

find first table3 where f1 = 1 and not  table3.f2 no-error. 
message available(table3). //OE: false; FWD true

Basically, the last query returns the record from FFCache, as the entry associated with that query wasn't evicted.
The root cause is FastFindCache.ReverseLookup.Value.invalidate. Here it is the initial code:

         private void invalidate(BitSet dirtyProps, LRUCache<Node, Value> lruCache)
         {
            for (Node node = head; node != null; node = node.next)
            {
               if (dirtyProps != null                  &&
                   node.key.k2.nonIndexedProps != null &&
                   !node.key.k2.nonIndexedProps.intersects(dirtyProps))
               {
                  continue;
               }

               if (node == head)
               {
                  head = node.next;
               }

               node.remove();
               lruCache.remove(node);

            }
         }
      }

ShowHide

=== modified file 'src/com/goldencode/p2j/persist/FastFindCache.java'
--- old/src/com/goldencode/p2j/persist/FastFindCache.java       2025-08-18 12:37:50 +0000
+++ new/src/com/goldencode/p2j/persist/FastFindCache.java       2025-12-15 15:39:14 +0000
@@ -1160,23 +1160,31 @@
           */
          private void invalidate(BitSet dirtyProps, LRUCache<Node, Value> lruCache)
          {
-            for (Node node = head; node != null; node = node.next)
+            Node node = head;
+            
+            while (node != null)
             {
-               if (dirtyProps != null                  &&
-                   node.key.k2.nonIndexedProps != null &&
-                   !node.key.k2.nonIndexedProps.intersects(dirtyProps))
+               Node nextNode = node.next;
+               
+               boolean shouldSkip = (dirtyProps != null
+                  && node.key.k2.nonIndexedProps != null
+                  && !node.key.k2.nonIndexedProps.intersects(dirtyProps));
+               
+               if (shouldSkip)
                {
+                  node = nextNode;
                   continue;
                }
-
-               if (node == head)
-               {
-                  head = node.next;
+               
+               if (node == head) {
+                  head = nextNode;
                }
-
-               node.remove();
-               lruCache.remove(node);
-
+               
+               node.remove();         
+               lruCache.remove(node); 
+               
+               // Advance the loop using the saved reference
+               node = nextNode;
             }
          }
       }

However, I will also look in other methods from there in order to understand if this pattern is used elsewhere.

1. Even though the entries from ReverseLookup.reverseCache will typically be removed when the Rec ids will not be hard referenced anywhere else, there is a problem for NO_RECORD. This is a class static filed, so it is always referenced, so the associated entries from ReverseLookup.reverseCache will never be deleted.
2. There is at least one execution path when records from ReverseLookup.lruCache are not deleted. We are only removing entries from there, when single records are invalidated. However, when the records for an entire dmo should be invalidated(like when called from FFCache.invalidate(int dmoUid, Integer multiplex, boolean remove)), they are not removed. This causes a real functional problem as in ReverseLookup.put(RecordIdentifier<String> recID, Map<L2Key, RecordIdentifier<String>> l3Cache, Key key) we check whether an entry already exists, so the entry is not inserted into reverse lookup, even though it is in the main cache from FFcache. This functional problem can be aleviated by performing some checks in ReverseLookup.put to understand if the existing entry is valid or not. However, I feel like this is not the right way. The main downside is that we will consume memory for entries which are not valid, maybe leading to problems in this regard. I feel like we need a mechanism for eagerly removing the entries exactly in the moment when the dmo is invalidated, however also providing good performance results. This should also reduce the number of records in this map. I came across #10776, where it was mentioned that calling get was slow on the map from ReverseLookup.

Alexandru Lungu wrote:

• Why do you need to actually check if nodeToRemove is inside the list? Can't you run nodeToRemove.remove() (and other operations related to head and lruCache@) directly? FYI, the list can be very large and iterating it may be very costly, considering that the nodes contain Key with actual arguments, FQL, etc.
  • If the node is not in the value, then the remove() is no-op, removal from lruCache is done reasonably and setting of head won't be possible.

nodeToRemove comes from ReverseLookup.put. There, a Node instance is created, however, only taking into account l3Cache and key. So, it doesn't have set the structures related to the linked list(prev and next). Therefore, only calling nodeToRemove.remove() won't actually remove it from the linked list. So, this is why I needed the exact instance that was already stored. I understand, however, the performance implications of this and I think that your solution from #10776 where we can access the Node based on the FastFindCacheEntry may come in handy.

I do not understand the changes in invalidate and why they are required. They seem to rewrite the for loop in a while loop without any functional change.

There is actually a slight functional change there:)). This is the diff for the change:

          private void invalidate(BitSet dirtyProps, LRUCache<Node, Value> lruCache)
          {
-            for (Node node = head; node != null; node = node.next)
+            Node node = head;
+            
+            while (node != null)
             {
-               if (dirtyProps != null                  &&
-                   node.key.k2.nonIndexedProps != null &&
-                   !node.key.k2.nonIndexedProps.intersects(dirtyProps))
+               Node nextNode = node.next;
+               
+               boolean shouldSkip = (dirtyProps != null
+                  && node.key.k2.nonIndexedProps != null
+                  && !node.key.k2.nonIndexedProps.intersects(dirtyProps));
+               
+               if (shouldSkip)
                {
+                  node = nextNode;
                   continue;
                }
-
+               
                if (node == head)
                {
-                  head = node.next;
-               }
-
-               node.remove();
-               lruCache.remove(node);
-
+                  head = nextNode;
+               }
+               
+               node.remove();         // Safe to disconnect now
+               lruCache.remove(node); // Remove from cache
+               
+               // Advance the loop using the saved reference
+               node = nextNode;
+            }
+         }

The problem was that the next property is accessed after node.remove(); is called. Inside node.remove, next and prev properties are reset. So, node.next will return null. In my changes I save the reference to node.next right at the beginning of the while loop, so I can access it after node.remove is called.

Alexandru Lungu wrote:

Andrei, please embed the changes from 10776 into 11037a and let me review and test it as a whole (according to #10776-9)

I'm on it, however, I don't think it is really ready for the review. Your changes from #10776 can allow a more performant solution as I said in the 1st paragraph from #11037-8. I will try to implement that.

I have ported the changes from 10776a into 11037a/rev. 16326.

I have committed 11037a/rev. 16327. I removed the part where I was deleting the stale nodes at put operation. I have changed into removing the records from the reverse lookup when the invalidation in the main cache happens. This was possible as after 10776a, I have the FastFindCacheEntry objects there.

I have retested the testcase from #11037-1 and #10776-10 and they are solved.

Alex, please review!