FWD » Core Development » Base Language

Please check every usage from p2j.oo, of MathOps and DynamicOps, so there is nothing like, for example, integer i1 = MathOps.plus instead of i1.assign(MathOps.plus).

I found multiple occurences of = MathOps. in our code.
In TotalAccumulator.accumulate:

      public void accumulate(BaseDataType datum)
      {
         total = MathOps.plus(total, (NumberType) datum);
      }

         int64 mod = MathOps.modulo(new int64(a), new decimal(b));
         return mod.isUnknown() ? null : mod.longValue();

if (op1 instanceof NumberType)
{
   r = MathOps.plus((NumberType) op1, (NumberType) op2);
}
//more code
return r;

         @// get the current loop control value
         val = var.get();

         // calculate the new loop control value
         if (num)
         {
            NumberType number = (NumberType) val;
            val = MathOps.plus(number, factor); 
         }
         else if (val instanceof datetime) // datetime-tz also
         {
            datetime dd = (datetime) val;
            val = datetime.plusMillis(dd, factor);
         }
         else if (val instanceof date)
         {
            date dd = (date) val;
            val = date.plusDays(dd, factor);
         }
         // else some incorrect looping datatype, probably should throw an exception

         // update the loop control var
         var.set(val);

In total I found the above construct about 15 times, but = TextOps. appears closer to 100 times and = CompareOps. again about 15 times. I can go thru and change these to an assign construct, but for example here I don t think it is necessary as the integer is not used int the next steps.

int64 mod = MathOps.modulo(new int64(a), new decimal(b));
return mod.isUnknown() ? null : mod.longValue();

As long as the Java variable assigned to that reference of the i.e. integerConstant is using it in a read-only way, is OK to leave it as-is. But it must not be returned to the application runtime, it should stay in the FWD runtime.

Seems pretty difficult to determine that for sure in all use cases. Using assign is the better way to go right?
It may be that I will spend more time determining if the value is used only in read only than just using assign.

Focus on what is obvious. We can always come back to something which stands up in profiling.

Committed on 9060a rev 15662, 15663 multiple uses of logical and int64 constants.
Tested on a large customer application, where during server startup, approximately 250k logical and 25k int64 instances are retrieved from cache. During a performance test run, an additional 1,000k logical and 250k int64 instances are reused.

Committed on 9060a rev 15664 multiple uses of character constants. These are used where the value has limited options.
Tested on a large customer application and during server startup we reuse 160k character.UNKNOWN, 60k character.EMPTY_STRING and 90k values retrieved from the cache. At this step the cache contains 17 values.
Then, during a performance test run we reuse an extra 5k character.UNKNOWN and 200k values retrieved from the cache. After this step the cache contains 31 values.

I am unsure if it is ok to use character.of in LegacyLogManager.getLogFileName() - this contains customer information but in theory is has a limited pool of values and it is used a lot of times, almost 60k times.

Ioana-Cristina Prioteasa wrote:

I am unsure if it is ok to use character.of in LegacyLogManager.getLogFileName() - this contains customer information but in theory is has a limited pool of values and it is used a lot of times, almost 60k times.

Go ahead and cache these. I think we should add a hard-limit to the character.of() cache to i.e. 2048 or something like this, for runtime.

Constantin Asofiei wrote:

Go ahead and cache these. I think we should add a hard-limit to the character.of() cache to i.e. 2048 or something like this, for runtime.

Got it, i will implement that next.

Committed to 9060a rev. 15665: limited character cache size, added directory.xml options to determine the character, integer and int64 cache sizes and some quick fixes.
What remains to be done here is checking where construct like integer i1 = MathOps.plus appear. I will move to that.

• static cache of configurable size.
  • it is an array where Nth position stores the int value for N.
  • this can be lazily initiated and the collection size is bound.
• dynamic cache of configurable size (lets say N).
  • it is an array with a bound size.
  • if an integer X is not in the static cache, then look into the dynamic cache. For this, compute X % N to gain a quick hash of X < N. PS: maybe use the absolute value to also cover negative values.
  • if there is something at the X % N position, then check if what is found is the right instance (e.g. dynamic_cache[x % n].value() == x % n).
    • if this holds, there you go ... cache hit
    • if not, then create a new instance and override the one from dynamic_cache[x % n].
  • if there is nothing at the X % N position, then ... cache miss. create a new instance and store it in dynamic_cache[x % n]

The gain of the dynamic cache: we still cache recently used large integers.
The downside of the dynamic cache: there is a low chance (depending on the collection size and collision probability) to trash the dynamic_cache on a very specific position.

Tested a big GUI customer application and a NPE showed up, caused by the use of CacheManager.
character.of was called before CacheManger.initialize, causing the cache in character to be null.
I think CacheManager was initially designed to initialize the persistence caches, as it's initialize method is called in Persistence.initialize. Since then, it's scope seemed to extend beyond persistence. I think it's a good idea for it to be refactored, add a server hook to initialize the CacheManager and modify it's configuration path from directory.xml from server/default/persistence to server/default.
Does this sound like a good approach?

After a quick fix for the NPE, the tested application showed no regressions, this is good news!

Ioana-Cristina Prioteasa wrote:

...and modify it's configuration path from directory.xml from server/default/persistence to server/default.
Does this sound like a good approach?

It makes sense, since the CacheManager is no longer used only for persistence. However, there are applications in production and testing which this change will break (well, cache configuration will be lost, which can have unexpected performance and memory consequences, as defaults are used).

To avoid using the hard-coded cache defaults, I think we need to prefer these new directory paths, but honor the old ones if they're found (and not overridden), logging a warning to alert admins that these settings are deprecated. Any new caches (like the one for this task) will only be read from the new location.

The cache configuration documentation will need updates as well (including a note about the deprecation of the old location).

Eric Faulhaber wrote:

To avoid using the hard-coded cache defaults, I think we need to prefer these new directory paths, but honor the old ones if they're found (and not overridden), logging a warning to alert admins that these settings are deprecated. Any new caches (like the one for this task) will only be read from the new location.

The initialization of cache sizes is handled in CacheManager, which searches for all containers inside /server/default/persistent/cache-sizes/ in the directory and builds a map of cache sizes based on all configurations found. When an actual cache is initialized in a specific class, it looks up its configured size in this map — if not found, it falls back to a default value.

The challenge here is that CacheManager does not inherently differentiate between old and newly added caches. Given this, the best approach seems to be:

1. First, initialize from /server/default/persistence. - Log a warning for each cache size found in this deprecated location.
2. Then, read from /server/default. - Any values found here will override those from the deprecated location.

At the end of this process, we will have cache sizes for all defined caches while also providing clear deprecation warnings for any values retrieved from /server/default/persistence.

The challenge here is that CacheManager does not inherently differentiate between old and newly added caches. Given this, the best approach seems to be:

I am not quite sure what you mean by old and new. Each time you demand a new cache from the CacheManager it will oblige, so I would say that these are always "new caches".

Alexandru Lungu wrote:

I am not quite sure what you mean by old and new.

"Old" (deprecated) path: /server/default/persistence/cache-sizes/
"New" path: /server/default/cache-sizes/

Got it - good approach in #9060-21 then.

I also agree with #9060-21, but make sure you update the wiki to reflect this change https://proj.goldencode.com/projects/p2j/wiki/Database_Configuration#Cache-Sizes, including the warnings when using the deprecated path.

Dănuț Filimon wrote:

I also agree with #9060-21, but make sure you update the wiki to reflect this change https://proj.goldencode.com/projects/p2j/wiki/Database_Configuration#Cache-Sizes, including the warnings when using the deprecated path.

We also need to migrate these paths in existing applications - maybe we can do this auto-magically, at FWD server startup?

• integer - 1024 (integer with values 0-1021 cached)
• int64 - 1024 (int64 with values 0-1021 cached)
• character - 2048 (the character cache cannot exceed 2048)

The overall execution time decreased by 2%, but certain steps in the performance test showed improvements of up to 13%. This is a significant result and the impact of these changes is clearly visible.

I will work next on the CacheManager and the dynamic cache Alex explained in #9060-17, maybe we can increase the impact of this even more.

• com.goldencode.p2j.util.int64 - Added int64Constant, static and dynamic cache using the CacheManager.
• com.goldencode.p2j.util.integer - Added integerConstant, static and dynamic cache using CacheManager
• com.goldencode.p2j.util.character - Used the CacheManager to instantiate the cache and added a hard limit to the size of the cache.
• com.goldencode.p2j.persist.dialect.P2JSQLServer2008Dialect - Added support for integer and int64 constants.
• com.goldencode.p2j.persist.hql.DataTypeHelper - Added support for integer and int64 constants.
• com.goldencode.p2j.persist.orm.types.TypeManager - Added support for integer and int64 constants.
• com.goldencode.p2j.util.BaseDataTypeFactory - Added support for integer and int64 constants.
• com.goldencode.p2j.util.DataSetSDOHelper - Added support for integer and int64 constants.
• com.goldencode.p2j.util.jobject - Added support for integer and int64 constants.
• com.goldencode.p2j.persist.CacheManager - added APIs for an array cache and an identity hash map cache and refactored to read the configuration first from /persistence/cache-size, register the sizes found there, but log a deprecated location warning and then check the new /cache-size location, add everything new found there and override the deprecated values if necessary.
• com.goldencode.p2j.main.StandardServer - added a server hook to initialize the CacheManager.
• com.goldencode.p2j.persist.Persistence - removed the initialization of the CacheManager.

• com.goldencode.p2j.oo.lang.LegacyClass in isA method.
• com.goldencode.p2j.persist.BufferFieldImpl in literalQuestion method.
• com.goldencode.p2j.persist.ConnectionManager in connectedImpl method.
• com.goldencode.p2j.persist.DataSource in addSourceBufferImpl method.
• com.goldencode.p2j.persist.DynamicConversionHelper in injectVariable method.
• com.goldencode.p2j.persist.DynamicTablesHelper in generateSchemaAst method.
• com.goldencode.p2j.persist.P2JQuery in getNext method.
• com.goldencode.p2j.persist.Presorter in isFirstOfGroup method.
• com.goldencode.p2j.persist.RecordBuffer in delete method.
• com.goldencode.p2j.persist.StaticTempTable in canUndo, clear and addFieldToIndex methods.
• com.goldencode.p2j.persist.TableMapper in isLegacyFieldMandatory and loadFields methods.
• com.goldencode.p2j.persist.TempTableBuilder in addNewField, @addNewIndex, addFieldToIndex, tempTablePrepareImpl createTableLikeImpl, addIndexLike and parseIndexString methods.
• com.goldencode.p2j.persist.meta.MetadataManager in populateTenant, childData, indexData, fieldData and build methods.
• com.goldencode.p2j.util.ArrayAssigner in assignMulti and assignSingle methods.
• com.goldencode.p2j.util.ErrorManager in isError method.
• com.goldencode.p2j.util.LegacyLogManagerImpl in writeMessage method.
• com.goldencode.p2j.util.NumberType in setNumericFormat method.
• com.goldencode.p2j.util.ProcedureManager in addSuperProcedureImpl and isPersistent methods.
• com.goldencode.p2j.util.SessionUtils in isErrorStackTrace method.
• com.goldencode.p2j.util.XDocumentImpl in load method.
• com.goldencode.p2j.util.XEntityImpl in getChild, removeChild and removeAttribute methods.
• com.goldencode.p2j.util.XNodeRefImpl in nodeValueToMemptr and normalize methods.

• com.goldencode.p2j.persist.AbstractTempTable in namespacePrefix method.
• com.goldencode.p2j.persist.RecordBuffer in delete method.
• com.goldencode.p2j.persist.meta.MetadataManager in addTableToFile, fieldData and build methods.
• com.goldencode.p2j.util.EnvironmentOps in getCurrentLanguage, getOperatingSystem and getRuntimeOperatingSystem methods.
• com.goldencode.p2j.util.HandleChain in name method.
• com.goldencode.p2j.util.HandleResource in resourceType method.
• com.goldencode.p2j.util.LegacyLogManagerImpl in getLogFileName method.
• com.goldencode.p2j.util.NumberType in getNumericFormat and setNumericFormat methods.
• com.goldencode.p2j.util.date in getDateOrder method.

• com.goldencode.p2j.persist.BufferFieldImpl in extent method.
• com.goldencode.p2j.persist.BufferImpl in numFields method.
• com.goldencode.p2j.persist.DataSet in numBuffers and bufferHandle methods.
• com.goldencode.p2j.persist.QueryComponent in resolveArg method.
• com.goldencode.p2j.persist.meta.MetadataManager in addTableToFile, childData, indexData, fieldData and build methods.
• com.goldencode.p2j.util.AccessorWrapper in get method.
• com.goldencode.p2j.util.ArrayAssigner in resize and lengthOf method.
• com.goldencode.p2j.util.DynamicOps in plus, minus and length methods.
• com.goldencode.p2j.util.TextOps in byteLength, indexOf, lastIndexOf, length, lookup and numEntries methods.
• com.goldencode.p2j.util.ToClause in 2 constructors.
• com.goldencode.p2j.util.date in getTimeZone method.
• com.goldencode.p2j.util.XEntityImpl in getNumChildren method.
• com.goldencode.p2j.util.XNodeRefImpl in getLocalName method.

• com.goldencode.p2j.persist.DynamicConversionHelper in injectVariable method.
• com.goldencode.p2j.persist.QueryComponent in resolveArg method.
• com.goldencode.p2j.persist.sequence.SequenceHandler in getResultValue, safeGetCurrentValue and safeGetNextValue methods.
• com.goldencode.p2j.util.ArrayAssigner in assignMulti, assignSingle, assignSingleUnknown and subscriptHelper methods.
• com.goldencode.p2j.util.BinaryData in length method.
• com.goldencode.p2j.util.CompareOps in _isEqual, isEqual, _isNotEqual, isNotEqual, _isGraterThan, isGraterThan, _isGreaterThanOrEqual, isGreaterThanOrEqual, _isLessThan, isLessThan, _isLessThanOrEqual and isLessThanOrEqual methods.
• com.goldencode.p2j.util.DynamicOps in plus and minus methods.
• com.goldencode.p2j.util.MathOps in abs, random, negate, plus, minus, multiply and modulo methods.
• com.goldencode.p2j.util.ToClause in 2 constructors.
• com.goldencode.p2j.util.date in secondsSinceMidnight and elapsed methods.
• com.goldencode.p2j.util.datetime in 2 constructors.
• com.goldencode.p2j.util.datetimetz in 4 constructors.

Also committed on 9060a rev. 15671 an empty row deleted by accident and some history entry revisions.
The revisions that need review on the branch: 15659-15671.
Constantin, please review! Thanks.

• CallParameter needs the integer/int64/characterConstant checks
• CacheManager.java
  • this message Please move it from /server/standard/persistence/cache-size to /server/default/cache-size. - keep in mind that you can have /server/default/ paths. I think the message should be like /server/{default|<serverID>}/persistent/cache-size path - same for the second, @/server/{default|<serverID>}/.
• character.java, int64.java, integer.java - there are some unused imports, please remove them
• character - this needs to be inside synchronized: if (CHARACTER_CONSTANTS.size() < CHARACTER_CACHE_LIMIT)
• int64.of and integer.of - do we need synchronization for DYNAMIC_CACHE?
• please run javadoc and fix any issues, but only in the files you've already changed

• 187k character.<init>(String)
  • 53k from Text.assign
  • 36k from CompareOps.isEqual / _isNotEqual / _isEqual
  • 12k from TypeFactory.character
• 101k character.<init>(character)
  • 96k from character.duplicate (BDT.copy, RAQ.resolveArg,
  • 4k from GenericFrame.parseValue
• 224k character.<init>(String, boolean, boolean>
  • 101k from TextOps.instantiate (toUpperCaseImpl, trimImpl, substringImpl, etc.)
  • 101k from Record._getCharacter
• 88k integer.<init>(Number)
  • 53k from Record._getInteger
  • 29k from UserTableStatUpdater.stat
• 81k integer.<init>(int)
  • 20k from TypeFactory.integer
  • 15k from TextOps.numEntries
  • 0.5k from FontTable.getTextWidthPixels
  • 0.2k from TextOps.lookup
  • 0.1k from FrameWidget.getDown
• 92k int64.<init>()
  • mostly from integer c'tors
  • 546 from FontTable.getTextWidthPixels
• 26k character.<init>()
  • mostly from run-time
  • 3.8k from StringFormat.fromVar4FieldValue
  • 0.5k from StringFormat.fromVar
• 38k int64.<init>()
  • 20k from CompareOps.isEqual
  • 9.4k from MathOps.minus
  • 3.9k from CompareOps._isEqual
  • 1.9k from CompareOps._isGreaterThanOrEqual
• 4k decimal.<init>(long)
  • 4.2k from MathOps.plus
• 4.2k decimal.<init>(BDT)
  • 3.7k from CompareOps.lessThanOrEqual
  • 0.4k from CompareOps.equals
• 17.5k logical.<init>(boolean)
  • 10k from P2JQuery.getNext()
  • 5.6k from TempTableBuilder.addNewField
  • 0.5k from HandleOps.canQuery
• 11k logical.<init>(Boolean)
  • 11k from Record._isLogical

Ioana, from the list you posted, I think the only part of concern is:

(/home/icp/projects/branches/9060a/src/com/goldencode/p2j/persist/FieldReference.java:1697)

but the call result = TextOps.toUpperCase(TextOps.rightTrim(c, " ")); will always return a new instance, is not using character.of.

Last commit in 9060a is from Feb 14 - please commit latest changes so we can get this into testing.

As a side note: in the spoon code you posted, you can further filter this for methods which return a BaseDataType; no need to change for now, just an idea if the list is in the 1000's of lines, then always look to reduce them, if possible.

Constantin Asofiei wrote:

• CallParameter needs the integer/int64/characterConstant checks

CallParameter contains the characterConstant changes and for int and int64 there is this check if (int64.class.isAssignableFrom(type)). I think this is fine?

The rest of the review is addressed and committed in 9060a rev. 15672.

Thanks, looks good. Lets get this into runtime testing.

I updated my list in #9060-32 (now is complete).

Alexandru Lungu wrote:

I updated my list in #9060-32 (now is complete).

TypeFactory instances can't be immutable/constants. The Record methods like Record._isLogical will be worked in a second phase.

Ioana: can you double-check Alexandru's list, to what extent this is covered by 9060a?

• TextOps.numEntries and TextOps.lookup
• CompareOps and MathOps
• P2JQuery.getNext()
• TempTableBuilder.addNewField()

• UserTableStatUpdater.stat - used integer.of
• FontTable - used int64.of in more functions
• FrameWidget.getDown - used integer.of
• HandleOps.canQuery - used logical constants

I will rebase the branch to facilitate testing, will update when that is done.

• This is an immutable integer instance.
• This is an immutable int64 instance.
• This is an immutable character instance.
• This is an immutable logical instance.

If they appear in the server.log, post it here.

There was again a NPE because the cache was not initialized in time. Was caused by the use of character.of in EnvironmentOps.getOperatingSystem(), it seems like that can get executed before the server hook initializes the CacheManager. Added a null check in character, committed on 9060a rev.15729.

I'm reiterating something Ioana and I discussed about character.of() - at this point in time, the size of this cache is limited, as the strings are interned. So, the domain of the string values used by character.of must be small and also be only from inside FWD runtime, and not from outside, from the application. We may want to try something different (like a LRU) in phase two, but this needs to be kept in mind as we add character.of to more and more places (for example, adding it to Record._getCharacter may do more harm than good).

OTOH, in phase 2 we want to see how we can leverage logical.of and maybe also integer.of - but maybe we should consider if is possible to delay creating the BDT for each record field, and keep Java values as long as possible.

The current implementation uses a simple array for the cache, correct? Keep in mind that an LRU cache will have a lot more overhead, if the cache currently uses only simple indexed access to the array. If the performance margins are slim for this improvement, the speed of the cache could have a big impact.

Eric Faulhaber wrote:

The current implementation uses a simple array for the cache, correct? Keep in mind that an LRU cache will have a lot more overhead, if the cache currently uses only simple indexed access to the array. If the performance margins are slim for this improvement, the speed of the cache could have a big impact.

I'm talking about character.of() which uses an IdentityHashMap as the cache (with a limited max size), so that's not an array. My point with LRU is to do some experiments, what happens if we allow more strings (not interned).

My point with LRU is to do some experiments, what happens if we allow more strings (not interned).

What about the same approach as the dynamic cache in #9060-17? Static array with hashCode as key. The only overhead will be the hashCode computation for the String if not already computed.

Alexandru Lungu wrote:

What about the same approach as the dynamic cache in #9060-17? Static array with hashCode as key. The only overhead will be the hashCode computation for the String if not already computed.

Yes, we can experiment in second phase; but on collision will require an equals.

Yes, we can experiment in second phase; but on collision will require an equals.

I checked H2 code right now and this technique is applied only for strings that do not exceed OBJECT_CACHE_MAX_PER_ELEMENT_SIZE in length (which defaults to 4096). Maybe we can do some experimenting and set a lower margin (?). If set to 16, I expect quite fast equals (also mind the == short-circuit).

Constantin, I believe Ioana said in the stand-up today that 9060a was ready for trunk. If you agree, please queue for merge.

9060a is merged to trunk as rev.15726 and archived.

Because of the large update I peek at the changes in r15726.

I think we can squeeze a bit more. There are cases where values like integer.of(0) and int64.of(1) are used. Using the of method and the lookup(s) can be avoided if for these particular cases we define constants. Not many of them, probably 0, 1, 2 for each of the classes is enough. A similar constant I saw already implemented for character.EMPTY_STRING.

At any rate, the way these constants are created makes me a bit nervous. These static constants of a subclass type cannot be created until the current class is fully loaded. So this puts the base for a recursive initialization or some kind of a deadlock. With JDK 8 and 17, it works, but this is a strange construct and possible to fail for other JDKs.

I added support for Constant BDTs for getters in Record. There was an issue with the type resolving in FuzzyMethodLookup, but it was fixed.
The code is in 9060b/15754.

Should every constructor for a BDT have an equivalent immutable() overload?

Artur Școlnic wrote:

Should every constructor for a BDT have an equivalent immutable() overload?

Add a static unmodifiable(<java native value>) to all BaseDataType sub-classes - get the list of all valid data type for a database or temp-table field (what the getter types in Record are).

What I meant is that we only use one or two constructor overloads in Record for each BDT, so it makes sense to have an equivalent (the same parameters) immutable method only for those and not for all the constructors.

Artur Școlnic wrote:

What I meant is that we only use one or two constructor overloads in Record for each BDT, so it makes sense to have an equivalent (the same parameters) immutable method only for those and not for all the constructors.

Ah, yes, that's good, add overloads as needed.

So we go with immutable or unmodifiable name for this new method?

Immutable objects can't be changed at all, unmodifiable objects can't be change externally, but their state can be modified internally.

Artur Școlnic wrote:

Immutable objects can't be changed at all, unmodifiable objects can't be change externally, but their state can be modified internally.

• if we generate a "wrapper" over an existing BDT, then we should go with unmodifiable, as it is a term used by Java collection wrappers (e.g. Collections.unmodifiableList). This intercepts calls to assign like a proxy and rules out any modification.
• if we generate objects of a new class that explicitly overrides the "assign" method, then immutable is the right terminology. However, there is absolutely no occurence of an "immutable" method in Java docs.
  • Java 9 onward defines of static method as a way to provide immutable variants, mostly like we attempted with FWD. Thus, my personal preference is to stick with of and use caching whenever possible. If this is not possible (like for characters), lets have a flag for that (i.e. honorStaticCache that will eventually do an intern and use the cache). The default for characters is honorStaticCache = false.

Alexandru Lungu wrote:

• Java 9 onward defines of static method as a way to provide immutable variants, mostly like we attempted with FWD. Thus, my personal preference is to stick with of and use caching whenever possible. If this is not possible (like for characters), lets have a flag for that (i.e. honorStaticCache that will eventually do an intern and use the cache). The default for characters is honorStaticCache = false.

I agree with this; lets go with .of() and an optional flag for i.e. character which would not cache the instance. This way, if we add future caches for types like date(time(tz)), then we don't need to change the Record getters.

Regarding the interference of this task to lazy hydration #6720.
IMO, at least for the current work on this task, we shouldn't have any conflicts with that. My only concern was if the data array from Record was directly accessed. Currently, it is not. However, Artur will probably need to access it in the future, and he will probably avoid using getDatum due to performance reasons. So, we should only take that into account when lazy hydration will get merged.

I will continue watching this task and if I see something that mixes with lazy hydration, I will notify about that.

• fuzzyMethodLookup - fromJava already has fromConstantType, I think toLegacyBDT call of fromConstantType will do nothing.
• fromConstantType - instead of 'indexOf', use 'endsWith', and after that a substring using the "Constant".length()
• why the access mode change for some methods in i.e. datetimetz?
• handle - instead of duplicating code in handleConstant for fromResourceId, get the common code into a method which returns a WrappedResource and call.
• rawConstant.setUnknown() - this breaks the 'immutable' part - if we cache them, then there will be problems.
• recid is missing the overrides to make it immutable
• look how logicalConstant is checked througout the project - same must be done for the new constant types.
• look into the implementations of DataHandler - at least persist.orm.types.NumericType.convert(Object) I think needs to create constants, too.
• why using recid.ofRecid?
• please place the public static 'of' method at the beginning of the file, after constructors

I have not checked if overrides are OK for all constant types.

Constantin Asofiei wrote:

Review for 9060b/15755:

• why the access mode change for some methods in i.e. datetimetz?

I needed the methods for constant types constructors.

• rawConstant.setUnknown() - this breaks the 'immutable' part - if we cache them, then there will be problems.

This is extensively used in FWD, not allowing it, breaks the project. For now, I do not have a solution.

• recid is missing the overrides to make it immutable

recid does not have any methods that alter it's state.

• why using recid.ofRecid?

There is a conflict with the of from integer.

Artur Școlnic wrote:

• rawConstant.setUnknown() - this breaks the 'immutable' part - if we cache them, then there will be problems.

This is extensively used in FWD, not allowing it, breaks the project. For now, I do not have a solution.

Please provide some examples.

Clearly a LRU cache is not a good idea for caching the BDT constants, but what about a Most Frequently Used (MFU) cache?
The insertion, access and eviction operations can be done in constant time O(1), the cache size can be configured to be a relatively small one, as to not consume too much memory. This would not be as quick as the current static caching done in integer, but is a viable solution for the more complex types.

Artur Școlnic wrote:

Clearly a LRU cache is not a good idea for caching the BDT constants, but what about a Most Frequently Used (MFU) cache?
The insertion, access and eviction operations can be done in constant time O(1),

MFU is a cache policy which doesn't imply a certain implementation. Please describe how is this going to be implemented: the data structures and algorithm involved.

the cache size can be configured to be a relatively small one, as to not consume too much memory.

Memory is the last one to care about if the performance is improved. We can afford to spend extra 1MB per server for 1% time improvement (on a consistent manner and without synchronization overhead).

This is the most efficient implementation I found:

• HashMap<K, V>: Stores the actual cache data.
• HashMap<K, Integer>: Tracks the frequency for each key.
• HashMap<Integer, LinkedHashSet<K>>: Groups keys by their access frequency. The frequency count is the key, and the values are sets of keys that have that frequency.
• minFrequency: Keeps track of the least frequently used frequency to easily find the entry to evict.

• get(K key):
  Retrieves the value for the specified key.
  Increments the access frequency of the key.
  Moves the key to the appropriate frequency list.
• put(K key, V value):
  Adds a new key-value pair to the cache.
  Initializes the frequency of the key to 1.
  Adds the key to the frequency list for frequency 1.
  Updates the minFrequency to 1 after insertion.
• Eviction:
  If the cache exceeds its capacity, it evicts the least frequently used entry (from the minFrequency list).
  The evicted entry is removed from both the cache and the frequency map.

The general idea is that the least used entries will always change, but the most used one will always be "in the top" of the cache, so if a lot of the same values are used, there won't be a lot of adding/eviction operations.

Artur Școlnic wrote:

This is the most efficient implementation I found:

I am not sure if this is more performant than a LRU cache, which uses a Map and a Linked List. The complexity of LRU operations is also O(1), but the practical constant is high (comparing to a simple array). We may agree that MFU can reach a similar constant to the LRU, but it has the same flaw as the LRU in the first place - which is high :)

For example, an arbitrary LRU operation is consuming 200 time units (which is O(1)) and the MFU operation is consuming 180 time units at best (which is also O(1)). In both cases, we may end up paying a time price higher than 20 time units that is memory allocation + GC stress. The #9060 goal is to check for a ... lets say ... 3 time units approach (which is again O(1)). Having this very narrow space forces us to use very trivial solutions. Very brute example: based on array look-up (2 time units) + one arithmetic operation (1 time unit). Simply invoking the get method, passing down parameters, hashing, returning from stack, etc. adds to the total of time units.

You can consider these "time units" as ms, opcodes executed, bytes accessed, or whatever time metric you see fit. All of the values in the example are simply illustrative and have no real experimentation behind - are there just to prove a point.

I did a memory profiling on a large application FWDTest and saw that the most instantiated BDTs are unknown, and there are not any actual highly used values that could be statically cached for a performance improvement. Most likely, the conversion change we discussed and the transition to UNKNOWN static constant in the project where it make sens, will decrease greatly the number of instantiated BDTs.

Memory profiling with the changes in 9060b results for BDT instances (large application FWDtest that uses a lot of CRUD operations):

object 1.4 million -> 300k (-78.57%)
datetimetz 15k -> 10k (-33.33%)
datetime 7.3k -> 5.8k (-20.55%)
date 33k -> 25k (-24.24%)
rowid 3.2k -> 2.1k (-34.38%)
handle 64k -> 50k (-21.88%)
decimal 280k -> 220k (-21.43%)
raw 5.8k -> 3k (-48.28%)

Depending on the scenario, BDT instance creation could be significantly reduced, I suppose most of it is due to the usage of UNKNOWN constants.

I committed rev 15765, character caching in Record was added, still, new instances are used in the getter as trying to use constants, even the UNKNOWN, brings huge regressions.

Artur Școlnic wrote:

I committed rev 15765, character caching in Record was added, still, new instances are used in the getter as trying to use constants, even the UNKNOWN, brings huge regressions.

Are you saying you are not using characterConstant for record's character getter?

At this point, no. I am working on it.

Currently I am trying to solve this issue:

 new InputOutputExtentField(tempTable, "field") // field is of character type

Maybe we can cast the constant types to normal ones in FieldReference.getObject?

Artur Școlnic wrote:

Maybe we can cast the constant types to normal ones in FieldReference.getObject?

Or pass a parameter to it, to indicate whether we want an immutable instance or not.

I think the place is in InputOutputExtentField.

And also for InoutOutputField?

Constantin Asofiei wrote:

And also for InoutOutputField?

It is possible, so far I have been solving issues as they arise.
So casting in such cases is ok?

Artur Școlnic wrote:

Constantin Asofiei wrote:

And also for InoutOutputField?

It is possible, so far I have been solving issues as they arise.
So casting in such cases is ok?

So there is no InputOutputField class. But, my point is: when you find a problem, look for the pattern: in this case, is INPUT-OUTPUT <field-ref>, which may be extent or not. Create standalone tests and check how it behaves.

For the InputOutputExtentField - yes, that is the place where to transform from constants to BDT. I don't see a simpler solution, as INPUT-OUTPUT means in 4GL that the argument is passed 'as reference' (be it a variable, field, property, etc).

Alexandru suggested that at some point we should create new mutable instances and return them in the case of InputOutputExtentField.

Artur Școlnic wrote:

Alexandru suggested that at some point we should create new mutable instances and return them in the case of InputOutputExtentField.

Yes, that's what I mean with 'transform from constants to BDT' - get an array with mutable instances instead of constants, in InputOutputExtentField.

I encountered a few issued when using constant character, one of them leaves some tests in a perpetual wait state, a code example is illustrated below, it is inspired by the code in one of our customer's test:

define temp-table tt1 fields f1 as char.
define temp-table tt2 fields f2 as char.
create tt1.
tt1.f1 = "100".
create tt2.
substring(tt2.f2,1,2) = right-trim(string(tt1.f1, "-999,999,999.99999")). 
message tt2.f2.

RecordBuffer.openScope(tt1, tt2);
tt1.create();
tt1.setF1(new character("100"));
tt2.create();
tt2.setF2(tt2.getF2().replace(rightTrim(valueOf(tt1.getF1(), "-999,999,999.99999")), 1, 2));
message((character) new FieldReference(tt2, "f2").getValue());

Although the result in FWD is the same as in 4GL, the tt2.setF2(tt2.getF2() code is troubling me. Now the replace is permitted in trunk, but I intend to restrict it in characterConstant, so in the future, code like that will fail. Most likely this will need to be changed at conversion.

As a side note, 130 attempts to use replace or replaceEntry with characterConstant are made by the FWDTests and some of them are failing due to this error.

Other than that, I managed to make it work and committed the code as rev 15766.

After running 106 FWDTests, these are the results of the dynamic cache usage:

95% of decimals are retrieved from the cache.
17% of the characters are retrieved from the cache. 
63% of the date are retrieved from the cache. 
50% of the datetimetz are retrieved from the cache. 
Other types have a low occurance or extremely high miss rate.

I committed the code that enables dynamic caching for decimalConstants as rev 15767.

Creating 1 mil datetimetz instances takes, on average, 6594 milliseconds, using the dynamic cache, that has a hit rate of 50%-60%, takes 9140 milliseconds, this is a 71% total execution time increase, so I did not include the dynamic caching for these types.

This is odd. Can you share the caching code for datetimetz?

 if (DYNAMIC_CACHE == null)
      {
         return new datetimetzConstant(odt);
      }
      // Check dynamic cache
      int index = Math.abs(odt.hashCode() % DEFAULT_DYNAMIC_CACHE_SIZE);

      synchronized (DYNAMIC_CACHE)
      {
         datetimetzConstant cachedInstance = DYNAMIC_CACHE[index];
         if (cachedInstance != null && odt.isEqual(cachedInstance.offsetDateTimeValue()))
         {
            return cachedInstance;
         }
         // Cache miss - create and replace the entry in dynamic cache
         datetimetzConstant newInstance = new datetimetzConstant(odt);
         DYNAMIC_CACHE[index] = newInstance;
         return newInstance;
      }

• CompareOps.equals can use a == short-circuit now, as there is a higher chance to have the same BDT as operands. For example tt.f1 = 1 can be short-circuited as the static cache is used. The same applies to tt.f1 = 1.1 as the dynamic cache is used.
• System.identityHashCode can be used for String to retrieve a unique identifier for them (similar to a hash-code, but I guess it is faster). As for the equals check we can use == instead. This way, we can provide the same character instances for same String instances. This can help for strings literals that are interned by default. I don't know the performance cost of identityHashCode ... this should be investigated.

Artur Școlnic wrote:

Of course, writing a performance test for such cases is not trivial, so there is a chance that the results look weird due to the testing methodology. I welcome any suggestions for testing the dynamic caching in isolation.

Test the cache directly in Java - create the datetime-tz strings (what is odt in your code?), save them in a list, and after that check how .of() behaves when giving it this list.

Constantin Asofiei wrote:

Test the cache directly in Java - create the datetime-tz strings (what is odt in your code?), save them in a list, and after that check how .of() behaves when giving it this list.

This is what I have been doing. The 70% performance decrease is the result of tests like that.

I am trying to replace all date instance creation in the project with the date.of. Naturally this breaks everything, should I leave the date instances that are used outside FWD as is?
For example:

date someDate = TypeFactory.date();// should return date or dateConstant?

Artur Școlnic wrote:

I am trying to replace all date instance creation in the project with the date.of. Naturally this breaks everything, should I leave the date instances that are used outside FWD as is?
For example:
[...]

TypeFactory and UndoableFactory must return mutable instances - this is how vars are defined in the converted 4GL code.

I suppose BaseDataTypeFactory.instantiate and any BDT generation method should return a mutable instance.

Artur Școlnic wrote:

I suppose BaseDataTypeFactory.instantiate and any BDT generation method should return a mutable instance.

Not necessarily. BaseDataTypeFactory is used only internally in FWD - are the usages requiring to have a mutable instance?

It seems so, I am getting errors in the SharedVariableManager because dateConstants are used.

The issue seem to originate from BaseDataType.sameType, it returns false for date and constant. Does it make sens to alter it so it returns true?

Artur Școlnic wrote:

The issue seem to originate from BaseDataType.sameType, it returns false for date and constant. Does it make sens to alter it so it returns true?

Why would a constant end up in the SharedVariableManager registries? That's the root cause IMO.

Object var = (extent != null ? Array.newInstance(expected, Math.max(0,  extent))
                                   : type != null ? new object(type) 
                                                  : BaseDataType.generateDefault(expected));

Artur Școlnic wrote:

[...]
generateDefault generates a date.UNKNOWN, that's why I asked about it.

Because of BaseDataTypeFactory, right? If so, leave BaseDataTypeFactory unchanged for now.

Replacing new date with date.of throughout the project means there are going to be a lot of overloads for the of, which means a lot of overloaded constructors for dateConstant. A lot of them use restricted methods (assign, setUnknown...), would it be acceptable to create private versions of these methods? This would mean duplication of code, but the alternative would be to duplicate it directly in the constructors, which also requires access to parent's attributes.

Artur, can you do a safe cut-out of the current changes you have to merge to trunk by EOW? Other matters can be deferred in the next iteration.
I really want the decimal dynamic cache (Urgent).

Here are my proposals for solving the replace issue:

original: tt2.setF2(tt2.getF2().replace(rightTrim(valueOf(tt1.getF1(), "-999,999,999.99999")), 1, 2));

option 1: tt2.setF2(new character(tt2.getF2()).replace(rightTrim(valueOf(tt1.getF1(), "-999,999,999.99999")), 1, 2));
// wrap the constant in a new char and use repalce on it.

option 2: tt2.setF2(tt2.getF2().substring(rightTrim(valueOf(tt1.getF1(), "-999,999,999.99999")), 1, 2));
//add a new method, substring returns a new immutable instance 

option 3: tt2.setF2(tt2.getF2().replace(rightTrim(valueOf(tt1.getF1(), "-999,999,999.99999")), 1, 2));
// check in the method if it was generated at conversion time, if true, allow altering the state of the constant. At 'runtime', throw the error, if that can be done...

The same approach would need to be applied to the replaceEntry method.

Constantin, Alexandru, I would appreciate your input on this.

Of course I will try to limit the changes to be applied only in the field access scenario.

I had a talk with Constantin, the conclusion was that we need to wrap in mutable instances the constant types for every assign style statement.

Review of 9060b:

• Leave a space between the target and cast type (e.g. datetimetz result = (datetimetz)cachedBDTs[offset]; should be datetimetz result = (datetimetz) cachedBDTs[offset];)
• _getCharacter doesn't seem to cache unknown values.
• There are spots in BaseRecord that still should invalidate the cache (e.g. copy(BaseRecord from), RecordMeta.applyInitialValues).
  • I think *Type.java classes are all changing the record's data array. For example, BlobType.setField is setting a field in the data and so it should invalidate the cached BDT.
  • Please look more in depth on this, I barely skimmed some places that need invalidation. Omitting some of this may be fatal as updates won't be visible as the cached value will be old.
• move static unknown constants at the top of properties (before the instance props).
• put overloaded methods one near each other (the one will less args before the one with more props): see character.of
• Isn't assign(value.val()); from clobConstant going to throw an error due to immutability?
• All .of variants may rather return UNKNOWN if the parameter is null. This better encapsulates the null-safety.
• Some braces are placed on the same line as the method signature (e.g. dateConstant(Date d) {)
• Misalignments:
  

  +   public static class datetimetzConstant
  +           extends datetimetz
  +           implements BaseDataTypeConstant

• Leftovers:
  

  long startTime = System.currentTimeMillis(); // Start time

• I am quite nervous with comparing double values with ==: cachedInstance.doubleValue() == value.doubleValue(). This may be not accurate due to limitations of double data type precision. Can we rather have cachedInstance.equals(value)? Please do the required tests to confirm we still have the same cache hit rate.
• I wonder if the synchronization is mandatory. What can happen if we remove that? The concurrent access is highly improbable on that and we pay a cost of synchronization for each decimal created. If there is actual concurrent access, if these are on different spots, I suspect nothing wrong can happen. If they are on the same position, a shadow update will happen so only one value will prevail in the cache, but two threads may work with two different instances. This is a very highly improbable scenario that will cost us only 1 extra instance creation. The cost of synchronizing however is too big!
  • Please do the required testing and if possible get rid of the synchronized block (from integer as well). Redo tests for date and other types you seem fit. Maybe the overhead was from synchronization.
• In object: res.assign(object.of(datum)); seems like the constant is redundant as it is assigned straight-away to a mutable instance that ends up being cached. Should the res simply be assigned with = operator to the of return value?

Constantin Asofiei wrote:

Review for 9060b rev 15768:

• in database_references.rules, I think you need to check for the index, too

I am not sure what you mean, the index of the property? Check it for what?

• character$characterConstant.equals was removed?

Yes, it was causing the unsupported exception. Is there a reason that characterConstant can't use the parent's method?

• look how logicalConstant or integerConstant is used - you need to register all new constant types in the same places

Do you mean replace new instance creation with constants in the project? If so, that would be the next step, which I already partly implemented, but for now we need to wrap up the existing changes.

Artur Școlnic wrote:

Constantin Asofiei wrote:

Review for 9060b rev 15768:

• in database_references.rules, I think you need to check for the index, too

I am not sure what you mean, the index of the property? Check it for what?

Check the position of the field property in the AST - it should be zero I think?

• character$characterConstant.equals was removed?

Yes, it was causing the unsupported exception. Is there a reason that characterConstant can't use the parent's method?

You mean Can not compare character with characer constant!? Or that now a constant and a mutable character can co-exist in the FWD runtime, so i.e. compare needs to be OK?

• look how logicalConstant or integerConstant is used - you need to register all new constant types in the same places

Do you mean replace new instance creation with constants in the project?

No, I mean where the i.e. logicalConstant.class is checked beside logical.class.

If so, that would be the next step, which I already partly implemented, but for now we need to wrap up the existing changes.

Is mandatory to make these changes in the branch, it can't be merged without them.

Constantin Asofiei wrote:

You mean Can not compare character with characer constant!?

Yes.

Is mandatory to make these changes in the branch, it can't be merged without them.

I tend to agree with this statement. As long as we throw "constant" BDT into the wild, we need to ensure they are properly supported by the rest of the FWD API. So indeed, jobject.toJavaInstance is one of the many examples.

Artur Școlnic wrote:

that would be the next step, which I already partly implemented, but for now we need to wrap up the existing changes.

I was talking about using the constant types in the project instead of creating new instances. Class checking of the BDTs though the project, indeed needs to be complemented with the new constant types checking, I will add this in the next revision.

Alexandru Lungu wrote:

• I am quite nervous with comparing double values with ==: cachedInstance.doubleValue() == value.doubleValue(). This may be not accurate due to limitations of double data type precision. Can we rather have cachedInstance.equals(value)? Please do the required tests to confirm we still have the same cache hit rate.
• I wonder if the synchronization is mandatory.

Removing the synchronization did not affect the hit rate much, but using cachedInstance.equals(value) simply breaks the caching because it is always false.

but using cachedInstance.equals(value) simply breaks the caching because it is always false.

1. why?
2. can you attempt compareTo() == 0

Isn't this expected? Comparing a decimalConstant with a BigDecimal using equals should not return true.

Oh, sorry for the confusion. I meant cachedInstance.value.equals(value) or something like that: comparing the BigDecimal of the cached instance with the BigDecimal provided, without converting to double and use ==.

Alexandru Lungu wrote:

Oh, sorry for the confusion. I meant cachedInstance.value.equals(value) or something like that: comparing the BigDecimal of the cached instance with the BigDecimal provided, without converting to double and use ==.

Only compareTo seems to work as expected.

Please make sure that compareTo returns 0 only when the scale is the same. Otherwise, we may end up considering def var x as decimal scale 2 initial 1 and def var y as decimal scale 3 initial 1 equal, although these should be different decimal instances.

decimal.compareTo uses BigDecimal.compareTo which checks the scale, so I think we are safe.

I started a more extensive testing process, hundreds of unit tests are failing because of assign style methods being used with a BDT that was retrieved with a getter, so it is a constant. The most notable example is clob.write. Should I wrap the constant given as a parameter in a new mutable type and return it instead?

Artur Școlnic wrote:

I started a more extensive testing process, hundreds of unit tests are failing because of assign style methods being used with a BDT that was retrieved with a getter, so it is a constant. The most notable example is clob.write. Should I wrap the constant given as a parameter in a new mutable type and return it instead?

Didn't we solved this via conversion? Did you reconvert with 9060b?

This is different, for example:

at com.goldencode.p2j.util.clob$clobConstant.assign(clob.java:705)
at com.goldencode.p2j.util.Text.assign(Text.java:2335)
at com.goldencode.p2j.util.clob.write(clob.java:443)
at com.goldencode.p2j.util.TargetLob.write(TargetLob.java:382)
at com.goldencode.p2j.util.LobCopy.run(LobCopy.java:333)

new LobCopy(new SourceLob(longchar), new TargetLob(new FieldReference(tt, "field"))).run(); 

This is just an example, the idea is to come up with a general approach for this kind of situations where the immutable types end up being used in FWD with state altering methods.

new LobCopy(new SourceLob(longchar), new TargetLob(new FieldReference(tt, "field"))).run();

I agree that public TargetLob(FieldReference field) should wrap the constant in a mutable instance as TargetLob.lob is an internal longchar used to store the destination value. On TargetLob.write, if a field exists, the field is set with this internal BDT! Thus, the lob property seems to be an "intermediate" place to store that.

Please do extra tests with memptr, blob and clob data types to ensure they are also properly handled by the LobCopy operation (after fixing the longchar case).

Artur, BTW, clobConstant needs to override all methods which write it, like the clob.write(clob.java:443).

Artur, please go through all Accessor.get() references - there only 64 I can find, and at least Stream.readField looks like it needs attention. And there, I don't understand this code:

            if (ref.getType() == blob.class)
            {
               var.assign(PropertyMapper.preprocessBlob(new character(var)));
            }
            else if (ref.getType() == clob.class)
            {
               var.assign(PropertyMapper.preprocessClob(new character(var)));
            }
            else
            {
               ref.set(var);
            }

ref is a field, but in case of blob or clob, the var gets assigned, insead of ref.set(var).

Artur Școlnic wrote:

I committed 9060b/15771, it contains fixes for issues from the review, constant type registering throughout the project and general regression fixing as a result of running a large application unit test suite.

Regarding public void readField(FieldReference ref), maybe I am mistaken, but I don't think it is used anywhere, otherwise I would have seen errors in it during testing. It looks like it calls itself recursively and no other methods call it.

See this test:

def temp-table tt1 field f1 as int.
create tt1.
import tt1.f1.

readField(FieldReference) is emitted at conversion.

The changes in rev 15771 should solve the immutable types problem

BaseDataType var = ref.get();
var = readField(var); // readField returns a muttable BDT

if (ref.getType() == blob.class)
{
   var.assign(PropertyMapper.preprocessBlob(new character(var)));
}
else if (ref.getType() == clob.class)
{
   var.assign(PropertyMapper.preprocessClob(new character(var)));
}
ref.set(var);

I believe clob/blob data should be preprocessed, assigned to var which then should be set to the field reference.

Please create some tests to check IMPORT with blob/clob.

I did the tests, with 9060b, both export and import is broken, with trunk, export is ok, but the import is broken. I am working on a fix.

So I think PropertyMapper.preprocessClob/Blob is necessary because it assigns to var the actual data of the lob, as opposed to ref.get() which assign the name/path of the lob file, so the preprocessing is vital, the problem is that it is broken, PropertyMapper.readLobData is failing because it relies on the ImportWorker to be initialized, but it isn't.

I see that Eric and Ovidiu worked on ImportWorker.initialize, can someone please explain where should it be called, it is needed to properly locate the lob files, which at this moment is not happening.

Artur Școlnic wrote:

I see that Eric and Ovidiu worked on ImportWorker.initialize, can someone please explain where should it be called, it is needed to properly locate the lob files, which at this moment is not happening.

It's used in the rules, check rules/schema/import.xml.

Dănuț Filimon wrote:

Artur Școlnic wrote:

I see that Eric and Ovidiu worked on ImportWorker.initialize, can someone please explain where should it be called, it is needed to properly locate the lob files, which at this moment is not happening.

It's used in the rules, check rules/schema/import.xml.

And ImportWorker must not be used by runtime

Well, PropertyMapper uses it:

   private static byte[] readLobData(String locator)
   {
      String path = ImportWorker.lobPath; // lobPath is null
      if (!path.endsWith(File.separator))

OK, then please create a task in the database project for this. The code may be used just for importing the .d data (and lobs), not for IMPORT statement at runtime.

• FieldReference
  • is missing history entry
  • do we need to do the same for blobConstant?
• RecordMeta
  • is missing history entry
• RecordLoader
  • is missing history entry
• characterConstant
  • you added javaSpacifyNull, but you always do value = javaSpacifyNull(value); if there is a value. This seems wrong.
• clobConstant
  • the if (BaseDataType.isProxy(value)) is always false, because you do Text value = new character(str);
  • why the checkUndoable(value); call? This is an immutable instance.
  • writeExternal should be allowed I think.
• decimal
  • DYNAMIC_CACHE is global per JVM. We need synchronization here, I don't think volatile is enough.
• DynamicOps
  • is missing history entry
  • what were the reasons for this change? Now you return the actual BDT instance (from the args) as the result, and not a copy; this is wrong.
• ObjectOps
  • I assume this change is related to delete object tt1.objectField; please do o.ref = null; only if the instance is an objectConstant.
    

             if (deleted)
             {
                o.ref = null;
             }
    

• recidConstant
  • shouldn't this override all methods as in integerConstant?
• TargetLob
  • is missing history entry
  • this.lob = new clob(field.get()); line is wrong. A field can be memptr also.
• Stream
  • what is the test for the readField(BaseDataType[] var) change?
• please run ant javadoc and fix any javadoc problems in the files changed in this task.
• data import needs to be tested

9060b rev 15772 contains some formatting changes.

Also, please rebase.

Rebased 9060b on trunk rev 15849.

In order to make the Stream.readField work with the constant types, I had to change the method implementation so that the result is no longer assigned to the argument variable, but a new BDT is created and returned, this solves some of the issued but creates other ones, for example:

UnnamedStreams.safeInput().readField(var);

var = UnnamedStreams.safeInput().readField(var);

Artur Școlnic wrote:

In order to make the Stream.readField work with the constant types, I had to change the method implementation so that the result is no longer assigned to the argument variable, but a new BDT is created and returned, this solves some of the issued but creates other ones, for example:
[...]
After the changes, the var will not hold the read value, in order to solve this issue, conversion changes need to be made so that code looks like:
[...]

This is not correct - you can't re-assign the BDT var created by the application, to another instance.

Alternatively, the changes I made to the readField can be reverted and another solution implemented, though I am not sure if one exists, if a constant is passed to it, we cannot change it's value.

Where can I find the changes to readField?

The changes are in 9060b/15859.

Artur Școlnic wrote:

The changes are in 9060b/15859.

Change the code in readField(BaseDataType) so that a new instance is created only and only if the parameter is a BaseDataTypeConstant. This should work.

Sounds good.

Question: please remind me when readField(BaseDataType[] var) is emitted.

Import statements, but I also saw code like:

import ^ newpath ^.

UnnamedStreams.safeInput().skipField();
UnnamedStreams.safeInput().readField(var);
UnnamedStreams.safeInput().skipField();
UnnamedStreams.safeInput().resetCurrentLine();

Artur Școlnic wrote:

Import statements, but I also saw code like:

I meant the array overload of readField

I can't see an obvious usage in the customer project i am working with.

I addressed the review and fixed a few bugs, I no longer have failing tests in the unit test modules I am using (> 5k tests).
When it comes to the DYNAMIC_CACHE in decimal, I removed the synchronization due to it's overhead, the retrieved value is checked anyway so there is no risk of retrieving a wrong one. I don't remember if I did extensive testing for this, but I could do it if needed.
I committed the changes to 9060b/15860.

• in Stream.readField, return res; should be used for non-constant case (to keep the existing behavior)
• for readField(BDT[]) - please try some examples with extent fields.
• TargetLob issue was not addressed (this.lob = new clob(field.get()))

Constantin Asofiei wrote:

• in Stream.readField, return res; should be used for non-constant case (to keep the existing behavior)

The existing behavior of readField is to reassign the value, it does not return anything. There is no need to return var.duplicate() for a non constant var.

• TargetLob issue was not addressed (this.lob = new clob(field.get()))

this.lob will have a mutable type constructed from a potentially immutable field reference.

Can I move on to more extensive testing?

Artur Școlnic wrote:

Constantin Asofiei wrote:

• in Stream.readField, return res; should be used for non-constant case (to keep the existing behavior)

The existing behavior of readField is to reassign the value, it does not return anything. There is no need to return var.duplicate() for a non constant var.

In readField(FieldReference) you have var = readField(var);. You are right that original code was not returning anything, but lets keep consistent and return var; even if is the same instance as the argument.

• TargetLob issue was not addressed (this.lob = new clob(field.get()))

this.lob will have a mutable type constructed from a potentially immutable field reference.

I don't understand. Original was:

   public TargetLob(FieldReference field)
   {
      this.field = field;
      this.lob = (LargeObject) field.get();
   }

   public TargetLob(FieldReference field)
   {
      this.field = field;
      this.lob = new clob(field.get());
   }

LargeObject instances which can be at a field are blob, clob, longchar and memptr. You are now limiting this constructor only for clob types.

Can I move on to more extensive testing?

Yes, after addressing above and rebase.

Does this.lob = (LargeObject) field.get().duplicate(); work, only and only if field.get() is a BaseDataTypeConstant?

Constantin Asofiei wrote:

Does this.lob = (LargeObject) field.get().duplicate(); work, only and only if field.get() is a BaseDataTypeConstant?

Should work, duplicate returns a mutable instance.

In TargetLob(FieldReference field), field.get() is called twice per call. Please use bdt for the second call.

Also, testing needs to include performance testing, where possible.

In fuzzyMethodLookup, the line:

System.out.printf("\n   CALLER %s\n   PARAMETERS: %d\n", node.dumpTree(true), caller.length);

fuzzyMethodLookup(methodName, sig, accessMode, isStatic, internal, null);

I don't understand, fuzzyMethodLookup is almost sure called. I see WARNING: more than one method def found using fuzzy looku in lots of projects.

Why do you get the NPE?

The call is fuzzyMethodLookup(methodName, sig, accessMode, isStatic, internal, null);
null is passed for the node parameter, and it is not reassigned, it simply could have not worked.
Previously, in fuzzyMethodLookup the method was returning earlier and was not reaching this point.

Artur Școlnic wrote:

The call is fuzzyMethodLookup(methodName, sig, accessMode, isStatic, internal, null);

Please post the stacktrace for this call.

Caused by: java.lang.NullPointerException
│  │  │  │  │                   at com.goldencode.p2j.uast.FuzzyMethodLookup.fuzzyMethodLookup(FuzzyMethodLookup.java:638)
│  │  │  │  │                   at com.goldencode.p2j.util.FuzzyMethodRt.lookup(FuzzyMethodRt.java:139)
│  │  │  │  │                   at com.goldencode.p2j.util.ControlFlowOps.resolveLegacyEntry(ControlFlowOps.java:6190)
│  │  │  │  │                   at com.goldencode.p2j.util.ControlFlowOps.resolveLegacyEntry(ControlFlowOps.java:5542)
│  │  │  │  │                   at com.goldencode.p2j.util.ControlFlowOps.resolveLegacyEntry(ControlFlowOps.java:4475)
│  │  │  │  │                   at com.goldencode.p2j.util.ObjectOps.invokePolyWorker(ObjectOps.java:860)
│  │  │  │  │                   at com.goldencode.p2j.util.ObjectOps.invokeStandalonePoly(ObjectOps.java:699)

Artur, on a second thought: this is a runtime stack - that location should never be reached. Something is wrong in 9060b.

 else if (list.isEmpty())
      {
         if (debug)
         {
            // if we still have ambiguous matches, display any stored error
            if (error[0] != null)
            {
               System.out.println(error[0]);
            }

            System.out.println("\n\n----------------fuzzyMethodLookup NO MATCH SUCKA!----------------\n");
         }

         return null; // ---> trunk
      }

      System.out.println("WARNING: more than one method def found using fuzzy lookup: " + name + 
                         " from class " + getClassName());

      System.out.printf("\n   CALLER %s\n   PARAMETERS: %d\n", node.dumpTree(true), caller.length); // ---> 9060b

With a simple null check, the failing tests are passing.

Artur, see FuzzyMethodRt.resolveSignature - type = arg.getClass().getSimpleName(); - if arg is a BaseDataTypeConstant, resolve the super-class. We already do this in ControlFlowOps.calculateType. But please review resolveLegacyEntry if there is any other missing place where BaseDataTypeConstant is not honored.

Constantin Asofiei wrote:

Artur, see FuzzyMethodRt.resolveSignature - type = arg.getClass().getSimpleName(); - if arg is a BaseDataTypeConstant, resolve the super-class. We already do this in ControlFlowOps.calculateType. But please review resolveLegacyEntry if there is any other missing place where BaseDataTypeConstant is not honored.

It did not solve the issue. It still looks wrong to me to pass a null value for node then calling dumpTree on it.

The point is: the type at that place most likely is a i.e. integerConstant - look further or otherwise debug and find why a constant type is being used in FuzzyMethodRt. This must not happen.

  ObjectOps.invokeStandalonePoly(Assert.class, "AreEqual", "III", new Object[]{actual.unwrapBuffer().bufferField(i).unwrapBufferField().value(), expect.unwrapBuffer().bufferField(fieldName).unwrapBufferField().value(), msg});

the arguments are integerConstant, but i don't think this is an issue, there are a lot of calls in FuzzyMethodRt with constant arguments that do not cause any issues. I reverted the changes made in FuzzyMethodLookup and the tests pass.

Artur, I understand that, but constant types should be resolved to their actual types. There is a bug somewhere in FuzzyMethodRt or resolveLegacyEntry. If we target can exactly on the type, a constant argument should not interfere with this.

Also, System.out.println("WARNING: more than one method .. you mention in FuzzyMethodRt is a debug output which until now was emitted only for conversion; for runtime, we need to include it in if (debug) processing (but conversion should still emit it, as its useful). In any case, please try first to find the 'root cause' why constant types are not resolved to their BDT super-class.

I committed the fix for this and other issues to 9060b/15882. Constantin, maybe you can take a look while I continue testing.

• you change the datum in setCompareMode, but the argument at the caller remains unchanged (thus the case-sensitivity is unchanged)
• also, I think is part of a larger problem: we have DataHandler.getField(), for which the returned instance can be assumed mutable in lots of places. I think we need to change getField to return a mutable instance, and not a constant, in all implementations.

Constantin Asofiei wrote:

• also, I think is part of a larger problem: we have DataHandler.getField(), for which the returned instance can be assumed mutable in lots of places. I think we need to change getField to return a mutable instance, and not a constant, in all implementations.

Or otherwise add a parameter to getField(boolean allowConstant), so that a mutable instance is created only in places where is needed; this can work only if these places can be identified.

Returning a mutable instance for all implementations seems to work fine, the testing passed.

OK, lets leave it as this for now.

• please do bzr diff -r 15867 --using meld and check the copyright year for all files
• run javadoc and fix problems only in the files changed in this branch

Otherwise, lets continue testing - all apps need to be nested, performance included.

Rebased 9060b on trunk rev 16051.

I extracted the more impactful and safe bdt caching code and tested the performance impact on a customer application. Lookup style requests were used for the experiment and I registered an average of ~5% performance regression with all apis affected in a negative way.