FWD » Core Development » Database

limit the number of cached DmoMeta instances, and when they are evicted, unload also from the AsmClassLoader

This will require a cache with lenient eviction policy.

• BufferManager.buffersByDMOBufClass registers temp-tables with null multiplex ID - this will not allow them to be processed or removed properly. I've changed this to use the DMO and the lookup to check for the multiplex ID match
• DmoMetadataManager.byId - this needs to be switched to a map and maintained properly. Alexandru: if you have some other customer heaps for long running FWD servers, please check this array's length in Eclipse MAT
• UniqueTracker - optimization for the no-op instance for temp-tables - there is no need for using dbMap
• SchemaDictionary - leak in tableIndexes, this should not be used for runtime conversion

TBD: for each dynamic temp-table, double-check the SQL H2 table name, if one is being created distinctly and it gets dropped properly.

TBD: to clean PropertyHelper's g2pCache, p2pgCache, p2psCache caches, SortIndex.cache and IndexHelper.cache. Some of them may be set to a LRUCache and with a size limit.

TBD: make the DMO registry a LRU cache and on evict clean up the artifacts/caches/etc

I think these is good news.

Constantin Asofiei wrote:

• DmoMetadataManager.byId - this needs to be switched to a map and maintained properly. Alexandru: if you have some other customer heaps for long running FWD servers, please check this array's length in Eclipse MAT

This is the array I was talking about in our meet today. Switching to a Map may be costly, and reusing the table IDs is not ideal.

However, there is another good news: I took a quick look at the usages of this, and the getDmoInfo() method (the only read from the data structure) is not used any longer. I think we can completely drop it now.

Note: in DmoMeta, there is a parallel id property, but it is (now) unrelated to the our table. It has its own idSeed generator and is actively used in many places. Even if these IDs may coincide, the byId is never used to search for the DMO.

Those being said, Alexandru, I think you can ignore Constantin's request.

DmoMetadataManager.byId - this needs to be switched to a map and maintained properly. Alexandru: if you have some other customer heaps for long running FWD servers, please check this array's length in Eclipse MAT

In MAT, I see byId with 3070 slots. Skimming, some of them are persistent tables, some are temp-tables, some a are dynamic temporary tables. I spotted dt200 the highest.

BTW: there are ~4376 temp DMOs and 958 persistent DMOs in the application profiled. So 3070 is not that much IMHO

Review for 11015a:

• Changes in BufferManager are dependent upon the unique pk across temp-tables; which I think was fixed in H2 in the mean-time, right? In other words, we can't afford to have these changes in BufterManager without the guarantee that the PK provided will find records only in the expected multiplex.
  • I am OK with the changes in SchemaDictionary at the price of performance drop.
  • However, cachedScopes was the main culprit in memory leaks when not clearing buffers. I expect this to accumulate as well. I think the key is dynamically computed.
  • The point is that we can also disable cachedScopes for a quick fix, but if we opt to keep it and assess how DMOs impls can be unloaded, then the same solution can be applied to tableindexes.
• UniqueTracker changes aim performance; good catch getDmoInfo was removed, so byld is removed. This is good.

Sorry for taking so long; I prepared myself for reviewing the DMO unload as well. Now I spotted that they are not ready yet. I will provide "my preparation" for review. It may be quite long:

• Prerequisite: We have a candidate for unloading only when the dynamic temp-table is deleted. This implies that its buffers are deleted which in turn make the queries and datasets invalid. This should happen now in trunk; if not, this should be fixed first.
• The "hard" relations toward the DMO impl are mostly Class for that class, BufferImpl/TemporaryBuffer for that DMO, DMOMeta and RecordMeta. There may be others, but I searched the project for classes that retain such information.
  • FieldReference retains DmoMeta and these can be referenced by converted code. These should be handled properly. I think they are quite intensively used internally, so this requires good testing.
  • RecordBuffer retains RecordMeta and DmoMeta. These should be marked as deleted. Buffers are not cached, so there is no evident leak here.
  • Queries may be retained by converted code (handles), so we need to ensure that these queries are closed completely, as components, sort criterions, etc. still hold the DMO. Internal queries should do the same (can't think of any yet). Queries are not cached AFAIK, so there are no evident leaks here.
    • SQLQuery objects are cached and rowStructure reference Class. I might think that the cache will eventually expire the queries with unloaded classes, but this should be tested.
    • FQLPreprocessor references dmoIfaces and is cached.
    • FQLHelper references meta and is cached.
    • FQLToSQLConverter references aliases (DmoMeta) and CompositeDmoInfo, which in turn stores parentDmoInfo. AFAIK, this is not cached.
  • Datasets: same story as queries.
  • Relations and joins: I found Relation annotation and RelatienJoe that retain Class. I am thinking of examples where datasets have both deleted and non deleted tables, so relations are broken. Joins are usually triggered by OF keyword and are retained by queries.
    • DmoMeta.putRelation can make a DMO meta reference another meta. The former one can be unloaded, so the DMOMeta of a still alive DMO may become corrupt.
  • serialization: JavaArgumentsParser, RecordSerializer.fromJson and RecordBufferSerializer either use Class or retain dmoMeta. I am not very familiar with these, but I can imagine that unloading of DMO can affect these if they occur after deleting a DMO impl.
  • deserialization: XmlExport and JsonExport cache objects that indirectly refer to the DMO impl: tableSchemas or tables. Unless we ensure these are initialized, used and deleted before the DMO impl is unloaded, then we are safe.
  • ChangeBroker may register several listeners that provide buffers which in turn reference unloaded DMOs. I can't tell for sure if there is something obvious to be done here; testing is deemed (change a buffer in a query with multiple components, out of which one has an unloaded class).
  • FastCopyHelper has bundles which are cached, which in turn reference dmo meta and Class
• UnsafePersistence intakes DMOMeta through direct-java access. We need to take care of customers that use unsafe persistence and/or public API with unloaded DMOs.
• can't tell what Record.fromPOJO is or whether it relates to unloading?
• can't tell if buffer arguments, binding and by-reference can be affected by unloading.
• other classes Record, PostCompilationOpsDriver, AssociationSyncher, ControlFlowOps, DataObjectFactory, TempTableResultSet, RecordMeta, TempTableHelper, TableMapper, LegacyTableInfo, DmoMetadataManager which still require analysis.
• OO classes may be affected by this unloading. I can imagine some standard 4GL OO classes that retain information about dynamic temp-tables (or use them), so some research should be done in this area.
• Class persisting (cold time optimization) is certainly affected by unloading; we need to decide which classes we need to persist to disk. Does unloading also evict from disk?

Alexandru Lungu wrote:

Review for 11015a:

• Changes in BufferManager are dependent upon the unique pk across temp-tables; which I think was fixed in H2 in the mean-time, right? In other words, we can't afford to have these changes in BufterManager without the guarantee that the PK provided will find records only in the expected multiplex.

There is no key which uses the PK in the buffersByDMOBufClass dictionary - and any check inside the scoped sets is done by also validating the multiplex ID for temp-tables, beside the PK.

• However, cachedScopes was the main culprit in memory leaks when not clearing buffers. I expect this to accumulate as well. I think the key is dynamically computed.

This is still under investigation.

• AsmClassLoader - seems ok
• CacheManager - is ok
• DynamicConversionHelper - seems ok
• DynamicTablesHelper - I spent quite a while in this class. I am mostly OK with the changes, but I need a bit more testing time with it.
• DmoMetadataManager
  • I think Map<String, DmoMeta> sMap = simpleRegistry.get(dmoMeta.getSchema()); should also be part of the synchronized black on delete in DmoMetadataManager.registerDmo; otherwise there can be a window of time in the delete process when registry doesn't have the DMO, but simpleRegistry has it.
  • dynamicRegistry should also be synchronized (?) from the same reasons as simpleRegistry? In other words, getDmoInfo should also wrap simpleRegistry work with a synchronized block, right?
  • All registry actions are syncrhonized - makes sense.
  • dynamicRegistry is not updated in the delete listener as this is called only on evict - makes sense.
• DmoMeta - is ok
  • I would prefer a sanity check in decrementUsage to avoid going from 0 to -1 when decrementing.
  • this.usage == -1 || this.usage != 0 this looks redundant. If it was made intentional, then it makes sense to stay like this for better clarity; if it is a leftover, then it can be adjusted.

This cleanup is not done immediately: it is postponed on the next top-level block exit. Classes will remain in memory until the caches which use them will evict their references, and at that point they will be garbage-collected. Also, the H2 SQL table for a temp-table will be dropped the same way (on next top-level block exit).

The eviction within the same-session happens when the top-level is exit, but relative to another session, the eviction can happen in a critical path? If session A and B coexist and session A evicts the DMO at the end of top-level, then from B POV it will happen at an arbitrary point of time, right? I am not saying something is wrong per-se, but I want to understand why the postpone is relevant for same-session, but irrelevant for cross-session. In other words, executeOnReturn is something fragile as it means the return of the procedure in the session in which the expiration happened ... which is completely non-deterministic anyway.

I am planning to continue reviewing the other classes as well and do some testing.

Review of 11015b, r16411

First, to note that all affected files lack the H entries and possible (c) year update. I'm sure they were skipped now to allow a smoother rebase process and the will be added at a later stage.

• DynamicTablesHelper
  • line 1271: the cast to TemporaryBuffer is not necessary because the getParentTable() method is defined in RecordBuffer. The test for rbuf.isTemporary() is also not required since the parent table is null for permanent tables, but it can be kept as an optimisation;
• DynamicTablesHelper
  • both getHibernateFieldName can be dropped. To my knowledge they are (no longer) in use;
  • createDynamicDMO: there are 3 calls to dmoMeta.incrementUsage(), but only one decrementUsage() (from TTB.forceDelete()). There is no clear pairing between these so I think there are some chances that the balance is affected causing the usage not to be properly decremented therefore making a DMO unable to be unloaded;
  • deregisterDmoUser() method: I think the cleanup is a bit over-engineered. A simple get() instead of getOrDefault() following by a null check would be a more efficient;
  • evict() method: IMHO, we do not need to panic and re-throw in case ClassNotFoundException.
  • dynamicRegistry: beside the 'natural' expiring of content, the entries are not removed on demand, when the DMOs are evicted. Shouldn't we do it instead of waiting the evicted DMOs to expire when the cache is full?
• DmoMeta
  • line 594: personally, I prefer 1L because on some fonts (Courier) 1l looks like 11;
  • line 649: usage -1 is a particular case of usage != 0. I think this should this should be rewritten as if (this.usage -1 || this.usage > 0) to better reflect the two situations: <static> || <dynamic-in-use>. As it appears on line 627.

I have not found any issue in the other modified files. At any rate, the code should work. The above notes are not show-stoppers but I think that possibly there might still be some leaks, as described.

I have no objections.
Constantin, I know I told you this week about some possible issues I seen while looking the first time at r16515 and newer, but after a second round my suspicions faded away. Sorry for the inconvenience.

Branch 11015b was merged to trunk rev 16546 and archived.