FWD » Core Development » Base Language

This is a separate issue created based on #7026-314.

• java.lang.ClassNotFoundException top 11 messages which is really strange:

  Message	Count
  com.goldencode.p2j.util.FieldReference	5258
  java.lang.FieldReference	5258
  java.lang.AdaptiveQuery	4270
  com.goldencode.p2j.util.AdaptiveQuery	4270
  com.goldencode.p2j.util.RandomAccessQuery	3980
  java.lang.RandomAccessQuery	3980
  java.lang.CompoundQuery	1160
  com.goldencode.p2j.util.CompoundQuery	1160
  java.lang.QueryConstants	404
  com.goldencode.p2j.util.QueryConstants	404
  com.goldencode.expr.CE4424	3

• java.lang.NoSuchMethodException top 11 messages:

  Message	Count
  com.goldencode.p2j.util.character.<init>(com.goldencode.p2j.util.logical)	5145
  com.goldencode.p2j.util.TextOps.toUpperCase(com.goldencode.p2j.util.character)	2378
  com.goldencode.p2j.util.BlockManager.toUpperCase(com.goldencode.p2j.util.character)	2378
  com.goldencode.p2j.persist.AbstractQuery.addComponent(com.goldencode.p2j.persist.RandomAccessQuery, java.lang.Integer, java.lang.Boolean)	1829
  com.goldencode.p2j.persist.CompoundQuery.addComponent(com.goldencode.p2j.persist.RandomAccessQuery, java.lang.Integer, java.lang.Boolean)	1829
  com.goldencode.p2j.persist.DynamicQuery.addComponent(com.goldencode.p2j.persist.RandomAccessQuery, java.lang.Integer, java.lang.Boolean)	1829
  java.lang.Object.addComponent(com.goldencode.p2j.persist.RandomAccessQuery, java.lang.Integer, java.lang.Boolean)	1829
  java.lang.Object.addComponent(com.goldencode.p2j.persist.AdaptiveQuery)	1173
  com.goldencode.p2j.persist.AbstractQuery.addComponent(com.goldencode.p2j.persist.AdaptiveQuery)	1173
  com.goldencode.p2j.persist.CompoundQuery.addComponent(com.goldencode.p2j.persist.AdaptiveQuery)	1173
  com.goldencode.p2j.persist.DynamicQuery.addComponent(com.goldencode.p2j.persist.AdaptiveQuery)	1173

The java.lang.ClassNotFoundException looks up classes that do not actually exist, while java.lang.NoSuchMethodException lookup up methods that do not exist.

Dănuț,

• in which cases imports contains null values? This array is initialized and populate in collectImportPaths(), using packageName.substring(). It will never return null.
• why null/unknown values are not allowed to be parameters of a constructor/method? I understand that what you did is rather an optimisation, by looking up first the constructor/method without null values, which should return quickly using the unique signature. The old resolution is performed only in cases where null values likely cause ambiguities;
• although the above point is clearly an optimization, I do not think it will decrease the ClassNotFoundException. Maybe just NoSuchMethodException s, which are internally caught for advancing to next overloaded method, but they should not be present in the log.

Ovidiu Maxiniuc wrote:

Dănuț,

I looked over your changes in r15367, Jast interpreter. I have a few questions here:

• in which cases imports contains null values? This array is initialized and populate in collectImportPaths(), using packageName.substring(). It will never return null.

You are right, the import path is actually "null." and not null, the value comes from the following compilation unit Aast:

compilation unit [COMPILE_UNIT]:68719476737 @0:0
   com.goldencode.p2j.util.* [KW_IMPORT]:68719476739 @0:0
   com.goldencode.p2j.util.BlockManager.* [STATIC_IMPORT]:68719476740 @0:0
   DynGenQuery [KW_CLASS]:68719476741 @0:0
       [CS_INSTANCE_VARS]:68719476745 @0:0
         = [ASSIGN]:68719476811 @0:0
            AdaptiveQuery [REFERENCE_DEF]:68719476812 @0:0
            AdaptiveQuery [CONSTRUCTOR]:68719476813 @0:0
       [CS_INSTANCE_METHODS]:68719476748 @0:0
         execute [METHOD_DEF]:68719476750 @0:0
             [BLOCK]:68719476751 @0:0
               initialize [METHOD_CALL]:68719476816 @0:0
                  query0 [REFERENCE]:68719476817 @0:0
                  bittTablePhysical [REFERENCE]:68719476818 @0:0
                  String [CAST]:68719476819 @0:0
                      [NULL_LITERAL]:68719476820 @0:0
                   [NULL_LITERAL]:68719476821 @0:0
                  bittTablePhysical._rowState asc [STRING]:68719476822 @0:0
                  WHOLE-INDEX,rowState [STRING]:68719476823 @0:0
   com.goldencode.p2j.persist.* [KW_IMPORT]:68719476777 @0:0
   null.* [KW_IMPORT]:68719476778 @0:0 <--------------------------------------------- here
   com.app.dmo._temp.* [KW_IMPORT]:68719476793 @0:0
   com.goldencode.p2j.persist.lock.* [KW_IMPORT]:68719476824 @0:0

• why null/unknown values are not allowed to be parameters of a constructor/method? I understand that what you did is rather an optimisation, by looking up first the constructor/method without null values, which should return quickly using the unique signature. The old resolution is performed only in cases where null values likely cause ambiguities;

• ControlFlowOps: This scenario receives Object... param, one of the parameter is a new unknown() (assigned to a handle in the external procedure right at the start, not modified further) so this unknown value is passed as a parameter and there is no method that has an unknown type instance in it's signature. Here the first two checks for the public/private method will fail, then it will later go through all the methods and determine that the new unknown() is actually a handle.
• SymbolResolver: A similar situation, for setters we don't actually know the type so we pass a signature containing a single UnknownType.class to later determine it's appropriate type. The type can also be null (ExpressionParser.method() calls ExpressionParser.generateSignature() which can end up adding a null value because of Verifier.resolveDataType() - this method has a switch case for the ast type and if the type is NULL it will return of course null). The comments left in the SymbolResolver.introspectFunction() also point out that null matching is done after attempting reflection (I don't think it should attempt it at all in this case).
• RuntimeJastInterpreter: This a similar situation to the one from SymbolResolver for null. Let me add that the "java.lang.*" import was moved and it's similar to staticImports which also adds the Math.class at the end, and that I should remove the null check for paths as the "null." was not hit in previous tests (with or without changes). I should look into how the "null.*" ends up as a KW_IMPORT, any tips on this?

• although the above point is clearly an optimization, I do not think it will decrease the ClassNotFoundException. Maybe just NoSuchMethodException s, which are internally caught for advancing to next overloaded method, but they should not be present in the log.

Danut Filimon wrote:

Right now I've done a similar test to the one from #7206-314 (10 runs + warmup) and managed to reduce the ClassNotFoundException from 68417 to 53345 and NoSuchMethodException from 53484 to 36435.

This is attributed entirely to moving the "java.lang.*" import path at the end, although I still have ClassNotFoundException caused by searching classes such as com.goldencode.p2j.util.FieldRefernce, com.goldencode.p2j.util.AdaptiveQuery, com.goldencode.p2j.util.RandomAccessQuery, com.goldencode.p2j.util.CompoundQuery, com.goldencode.p2j.util.QueryConstants which are actually from the persist package.

Dănuț Filimon wrote:

Ovidiu Maxiniuc wrote:
You are right, the import path is actually "null." and not null, the value comes from the following compilation unit Aast:
[...]

This is nonsense. Please find the TRPL which generates this and fix the issue there. We need to eliminate the bad annotation, not to recover after this problem.

I am not talking about the value of the parameter for a method, I am talking about the type of the parameter. Let me clarify these cases:

Of course. The presence of null values makes it impossible to know the type of the parameter on that position.

• ControlFlowOps: This scenario receives Object... param, one of the parameter is a new unknown() (assigned to a handle in the external procedure right at the start, not modified further) so this unknown value is passed as a parameter and there is no method that has an unknown type instance in it's signature. Here the first two checks for the public/private method will fail, then it will later go through all the methods and determine that the new unknown() is actually a handle.

Probably the static context cannot provide the actual type for a _poly method/function/method. At runtime you know it should be a handle, but when conversion runs, the only solution is to use the unknown class.

• RuntimeJastInterpreter: This a similar situation to the one from SymbolResolver for null. Let me add that the "java.lang.*" import was moved and it's similar to staticImports which also adds the Math.class at the end, and that I should remove the null check for paths as the "null." was not hit in previous tests (with or without changes). I should look into how the "null.*" ends up as a KW_IMPORT, any tips on this?

Yes, as noted above. you can breakpoint the places where the import JASTs are created (KW_IMPORT and STATIC_IMPORT).

This is attributed entirely to moving the "java.lang.*" import path at the end, although I still have ClassNotFoundException caused by searching classes such as com.goldencode.p2j.util.FieldRefernce, com.goldencode.p2j.util.AdaptiveQuery, com.goldencode.p2j.util.RandomAccessQuery, com.goldencode.p2j.util.CompoundQuery, com.goldencode.p2j.util.QueryConstants which are actually from the persist package.

OK. I understand. Here is an idea of how this can be avoided. We can keep a static map with hints for the package (maybe better the .class) of the most used class names, like FieldRefernce, AdaptiveQuery, etc. Before iterating the imports we can use this map quickly identify the correct class/package. However, this is a highly heuristic approach and only the name of the classes which we are sure are unique should be hinted. This can actually improve the resolution performance.

I've identified where the "null.*" is added, it comes from CE3124 (I remember seeing this type of class a lot), the stack trace:

ConverterHelper.createImport(String, boolean, JavaAst, AstSymbolResolver) line: 174    
ConverterHelper$Helper.createImport(String) line: 217    
CE3124.execute() line: not available [local variables unavailable]    
Expression.execute() line: 380    
Rule.apply(AstSymbolResolver, int) line: 500    
Rule.executeActions(AstSymbolResolver, ArrayList<RuleListElement>) line: 751    
Rule.coreProcessing(AstSymbolResolver, Object) line: 717    
Rule.apply(AstSymbolResolver, int) line: 537    
RuleSet(RuleContainer).apply(AstSymbolResolver, int) line: 597    
RuleSet.apply(AstSymbolResolver, int) line: 98    
RuleSet(RuleContainer).apply(AstSymbolResolver, int) line: 597    
RuleSet.apply(AstSymbolResolver, int) line: 98    
AstWalker.walk(AstSymbolResolver, Iterator<Aast>) line: 262    
AstWalker.walk(AstSymbolResolver, Aast, int) line: 210    
PatternEngine.apply(RuleSet, Aast) line: 1692    
PatternEngine.processAst(Aast) line: 1578    
PatternEngine.run() line: 947    
ConversionPool.runTask(ConversionProfile, ConversionPool$Results, Aast...) line: 346    
ConversionPool.runTask(ConversionProfile, Aast...) line: 301    
DynamicQueryHelper.lambda$parse$10(ProgressAst) line: 523    
239296655.exec() line: not available    
NanoTimer.timer(Operation) line: 131    
DynamicQueryHelper.parse(QueryProcessor, ArrayList<Buffer>, String, String, List<Buffer>) line: 523    
DynamicQueryHelper.lambda$parseQuery$12(P2JQuery[], DynamicQueryHelper$QueryProcessor, ArrayList, String, String, List) line: 1017    
796042721.exec() line: not available    
NanoTimer.timer(Operation) line: 131    
DynamicQueryHelper.parseQuery(ArrayList<Buffer>, String, String, List<Buffer>) line: 1017    
QueryWrapper.prepare(character) line: 4478    

The "null.*" comes from rules/convert/buffer_definitions.rules line 703.

Dănuț Filimon wrote:

The "null.*" comes from rules/convert/buffer_definitions.rules line 703.

This is the problematic rule. In the case where the second action is executed, the pkg does not have any value as the only appearance is only it being declared so it makes sense for it to be null when the getNoteString("pkgname") is also null.

         <!-- handle buffer-specific import statement -->
         <rule>
            isNote("is_dynamic_table") and getNoteBoolean("is_dynamic_table")
            <action>pkg = "com.goldencode.p2j.persist.dynamic._temp.*"</action>
            <action on="false">pkg = sprintf("%s.*", getNoteString("pkgname"))</action>
         </rule>
         <action>createImport(pkg)</action>

Dănuț Filimon wrote:

[...] Not sure where this CE3124 comes from?

The TRPL code is assembled into Java code. This is one of the classes generated on-the-fly.
The condition field from Rule.apply stack element should give you the exact TRPL rule being executed.

Dănuț Filimon wrote:

The "null.*" comes from rules/convert/buffer_definitions.rules line 703.

This is the problematic rule. In the case where the second action is executed, the pkg does not have any value as the only appearance is only it being declared so it makes sense for it to be null when the getNoteString("pkgname") is also null.
[...] I should dig further into why is the pkgname null.

It appears that pkgname is NOT set for a specific node. What is the node when "null.*" import is created?

The jroot JavaAst when calling createImport() is:

compilation unit [COMPILE_UNIT]:68719476737 @0:0
   com.goldencode.p2j.persist.dynquery [KW_PACKAGE]:68719476738 @0:0
   com.goldencode.p2j.util.* [KW_IMPORT]:68719476739 @0:0
   com.goldencode.p2j.util.BlockManager.* [STATIC_IMPORT]:68719476740 @0:0
   DynGenQuery [KW_CLASS]:68719476741 @0:0
       [CS_CONSTANTS]:68719476742 @0:0
       [CS_STATIC_VARS]:68719476743 @0:0
       [CS_STATIC_INITS]:68719476744 @0:0
       [CS_INSTANCE_VARS]:68719476745 @0:0
       [CS_CONSTRUCTORS]:68719476746 @0:0
       [CS_STATIC_METHODS]:68719476747 @0:0
       [CS_INSTANCE_METHODS]:68719476748 @0:0
         execute [METHOD_DEF]:68719476750 @0:0
             [BLOCK]:68719476751 @0:0
                [BOGUS]:68719476752 @0:0
                [SKIP]:68719476753 @0:0
               externalProcedure [STATIC_METHOD_CALL]:68719476754 @0:0
                  DynGenQuery.this [REFERENCE]:68719476755 @0:0
                  Block [CONSTRUCTOR]:68719476756 @0:0
                     Pre [CAST]:68719476757 @0:0
                         [LAMBDA]:68719476758 @0:0
                            [LPARENS]:68719476759 @0:0
                           block [BLOCK]:68719476760 @0:0
                     Init [CAST]:68719476761 @0:0
                         [LAMBDA]:68719476762 @0:0
                            [LPARENS]:68719476763 @0:0
                           block [BLOCK]:68719476764 @0:0
                     Enter [CAST]:68719476765 @0:0
                         [LAMBDA]:68719476766 @0:0
                            [LPARENS]:68719476767 @0:0
                           block [BLOCK]:68719476768 @0:0
                     Body [CAST]:68719476769 @0:0
                         [LAMBDA]:68719476770 @0:0
                            [LPARENS]:68719476771 @0:0
                           block [BLOCK]:68719476772 @0:0
                     Fini [CAST]:68719476773 @0:0
                         [LAMBDA]:68719476774 @0:0
                            [LPARENS]:68719476775 @0:0
                           block [BLOCK]:68719476776 @0:0
       [CS_INNER_CLASSES]:68719476749 @0:0
   com.goldencode.p2j.persist.* [KW_IMPORT]:68719476777 @0:0
   null.* [KW_IMPORT]:68719476778 @0:0

If you add -Drules.tracing=true at the FWD server, I think the runtime conversion will emit the by-rule annotations, which can help you find where the null.* import is emitted. So you can do a root.dumpTree(true) to get the full annotations.

Dănuț Filimon wrote:

The jroot JavaAst when calling createImport() is:
[...]

I was not referring to the JAST, but to this Progress AST, at the moment when the KW_IMPORT:68719476778 is created.

Indeed, -Drules.tracing=true will reveal the point where the node is created.
Unfortunately, will not help you with the core issue. It will trace things which have been done, in this case we are missing a pkgname annotation.
However, inserting a printfln("%s", copy.dumpTree(true)) will reveal the node type/text and might help understand whet is wrong with this node.

Also, about the ErrorConditionException and NO-ERROR mode: I don't think we can aggressively change this so that the ErrorConditionException is not being thrown, and instead the logic knows to 'return safely' from the call, without side-effects. For example, it is assumed that each and every recordOrShowError or recordOrThrowError is followed by a Java return, but (depending on how deep in the call for this i.e. DELETE OBJECT operation we are), this does not mean that the DELETE will stop right there and there if no exception is thrown. That's why I think first is to identify which are the most important cases, and address them one-by-one. For example, even for DELETE OBJECT, it depends on the target resource.

Greg: the point of the above is to not raise an ErrorConditionException when in silent mode, but gracefully 'abort' the operation without the exception, and let the stack safely unwind without side-effects.

Also, there was another idea mentioned by Alexandru: for FIND ... NO-ERROR cases, instead of emitting the silent(() -> lambda, emit a i.e. silentFirst() which avoids the lambda and lets the code know this is a NO-ERROR code and (considering the previous above point) also not rely on ErrorConditionException to know that the call failed.

Ovidiu Maxiniuc wrote:

However, inserting a printfln("%s", copy.dumpTree(true)) will reveal the node type/text and might help understand whet is wrong with this node.

I tried to do this in the rules, but I am not sure where this output of the printfln is displayed.

Dănuț Filimon wrote:

Ovidiu Maxiniuc wrote:

However, inserting a printfln("%s", copy.dumpTree(true)) will reveal the node type/text and might help understand whet is wrong with this node.

I tried to do this in the rules, but I am not sure where this output of the printfln is displayed.

printfln will be in the FWD server's log, but the logging level is FINE, so unless you adjust it, you will not see it.

Also, about the ErrorConditionException and NO-ERROR mode: I don't think we can aggressively change this so that the ErrorConditionException is not being thrown, and instead the logic knows to 'return safely' from the call, without side-effects. For example, it is assumed that each and every recordOrShowError or recordOrThrowError is followed by a Java return, but (depending on how deep in the call for this i.e. DELETE OBJECT operation we are), this does not mean that the DELETE will stop right there and there if no exception is thrown. That's why I think first is to identify which are the most important cases, and address them one-by-one. For example, even for DELETE OBJECT, it depends on the target resource.

Greg: the point of the above is to not raise an ErrorConditionException when in silent mode, but gracefully 'abort' the operation without the exception, and let the stack safely unwind without side-effects.

I agree this would be quite risky and it would be needed in a very large number of places.

One idea: have a configurable option to throw a single pre-constructed instance (per context) of ErrorConditionException instead of a newly constructed instance. We achieved good results in the past with this idea and it can be optional, so if we need the stack traces, we can always configure it for that.

Greg Shah wrote:

One idea: have a configurable option to throw a single pre-constructed instance (per context) of ErrorConditionException instead of a newly constructed instance. We achieved good results in the past with this idea and it can be optional, so if we need the stack traces, we can always configure it for that.

This is interesting, isn't this something similar that can be done to DeferredLegacyErrorException? Most of the time Each time, this instance is initialized with super(-1, null); which will also end up initializing a NumberedException with the same message and error code number (null, -1).

• NumberedException: Message null, Count 53133/1392278, Top 12
• Exception: Message null, Count 59400/59400
• DeferredLegacyErrorException: Message null, Count 53133/53133
• NumberFormatException: Message null, Count 34543/34543

Yes, we do this already. Any exception that doesn't need a stack trace is a candidate for this "trick". It saves a LOT of CPU.

• Reduce the number of DeferredLegacyErrorException by relying on a single instantiated objected in ErrorManager.
• Reduce the number of NumberFormatException being thrown by checking if the node exists first in XmlImport.
• Revert null import path check, the null was actually a "null.*" (string), but it was not a checked path as it would find com.goldencode.p2j.persist import path as the match before it even reached it.

I tested a customer scenario, 100 runs with 9032a/15268 and com.goldencode.p2j.util.DeferredLegacyErrorException does not appear in the Exceptions at all and java.lang.NumberFormatException has been reduced from 34k to a single exception.

Ovidiu Maxiniuc wrote:

Dănuț Filimon wrote:

This is attributed entirely to moving the "java.lang.*" import path at the end, although I still have ClassNotFoundException caused by searching classes such as com.goldencode.p2j.util.FieldRefernce, com.goldencode.p2j.util.AdaptiveQuery, com.goldencode.p2j.util.RandomAccessQuery, com.goldencode.p2j.util.CompoundQuery, com.goldencode.p2j.util.QueryConstants which are actually from the persist package.

OK. I understand. Here is an idea of how this can be avoided. We can keep a static map with hints for the package (maybe better the .class) of the most used class names, like FieldRefernce, AdaptiveQuery, etc. Before iterating the imports we can use this map quickly identify the correct class/package. However, this is a highly heuristic approach and only the name of the classes which we are sure are unique should be hinted. This can actually improve the resolution performance.

I really wanted to come back to this note from #9032-5 and say that I like the idea, but I don't think this is the right thing to do as we will need to keep track of this in the future when we make changes to the respective classes or add new ones. I'd rather try finding where the KW_IMPORT package for com.goldencode.p2j.util.* is added and maybe put com.goldencode.p2j.persist.* before it.

Discussed during yesterday's meeting, relying on other exceptions to fill the callStack of the error for a LegacyErrorException is not possible as the exceptions do not have any information that can be traced back to the callStack as it is populated only when the LegacyErrorException is created.

Ovidiu, do you have any small example that will reach createImport()? I've tried to create one similar to one of the patterns used in the application (before buffer, dynamic query) but I did not manage to reproduce it.

Something I thought of based on #9032-17, we can have a single ErrorConditionException instance that can be thrown for each "fixed" error message. By fixed, I mean a message that does not require formatting tokens (replacing %1, %2 and so on). There's a list of such cases in ErrorManager.replaceTokens(), for example:

         case   143: msg = "** %1: Unable to evaluate field for assignment"; break; <- token needs to be replaced, exception message will not be the same
         case   223: msg = "** Incompatible data types in expression or assignment"; break; <- no tokens, exception message will be the same for each created instance

Dănuț Filimon wrote:

Something I thought of based on #9032-17, we can have a single ErrorConditionException instance that can be thrown for each "fixed" error message. By fixed, I mean a message that does not require formatting tokens (replacing %1, %2 and so on). There's a list of such cases in ErrorManager.replaceTokens(), for example:
[...]

Just a background first:
The content of the values of the switch was extracted from 4GL GUI. In Help menu of the procedure editor, the last two entries are: Messages... and Recent Messages.... The former allows the user to lookup a specific error message, the latter lets you navigate to latest errors encountered by your application. The tokens can be seen in <>. The final idea for this was to have the switch morphed into something else (map/table) which would allow easier translation of messages.

Back to your idea: indeed, the errors are (if not, they probably should be) immutable objects. Based on this observation, if I understand correctly, you hope that some kind of prototype for those which have no parameters to be re-used each time such an error is needed. This would avoid short-lived creation of all instances for the specified ID and probably will have some performance benefits.

However, the problem for the parametrised errors still remains. And those form the majority. Even I tried to optimize as much as possible the token replacement algorithm, this might be still a problem. I am thinking of delaying this operation, for as much as possible. Working in lazy mode: keep the error ID and the table of replacements in the object, but only 'assembling' the final message only if it is required. Since the error check is normally performed using the ID and the message is printed only in exceptional cases, the replaceTokens() will not be executed.

Note that not errors are raised using this relatively new API. There are lots of places where the message is inlined (using + concatenation or local String builders) or even worse, using String.format(). The latter has a huge impact, since it will use advanced pattern matching for identifying placeholders and, as noted above, the resulted value (message String) will never be used.

Dănuț Filimon wrote:

Ovidiu, do you have any small example that will reach createImport()? I've tried to create one similar to one of the patterns used in the application (before buffer, dynamic query) but I did not manage to reproduce it.

Using a built-in 4GL function which FWD implemented in a different package should do the trick.
Creating a dynamic query for the FILL or SAVE-CHANGES methods in a Dataset with the DataSource from a different (permanent) database should add the imports needed for that related DMO.

Ovidiu Maxiniuc wrote:

Back to your idea: indeed, the errors are (if not, they probably should be) immutable objects. Based on this observation, if I understand correctly, you hope that some kind of prototype for those which have no parameters to be re-used each time such an error is needed. This would avoid short-lived creation of all instances for the specified ID and probably will have some performance benefits.

However, the problem for the parametrised errors still remains. And those form the majority. Even I tried to optimize as much as possible the token replacement algorithm, this might be still a problem. I am thinking of delaying this operation, for as much as possible. Working in lazy mode: keep the error ID and the table of replacements in the object, but only 'assembling' the final message only if it is required. Since the error check is normally performed using the ID and the message is printed only in exceptional cases, the replaceTokens() will not be executed.

Note that not errors are raised using this relatively new API. There are lots of places where the message is inlined (using + concatenation or local String builders) or even worse, using String.format(). The latter has a huge impact, since it will use advanced pattern matching for identifying placeholders and, as noted above, the resulted value (message String) will never be used.

You are right. Assembling the final message sounds like a good idea, but there's the problem of errors messages built with concatenation or String.format() which doesn't make use of this change. I think the right thing to do is to get rid of this type of operations from error building first so that we are able to get closer to using the new API and then focus on the next steps that can be taken.

The table contains the first and last 5 runs, with and without the 9032a changes. There's a 500 ms improvement for the first run, then is starts to decrease to around 100ms for the following runs. But the last 5 runs are showing a worse time with 9032a (200-300ms).

I've made a few adjustments to reduce the search for null parameter types. Instead of using Arrays.stream(parTypes).anyMatch(v -> v == null);, it is more favorable to check for them while collecting the parameters so that we are aware that there are null values from the start.

Committed 9032a/15369. Removed usage of Arrays.stream(), incorporated the functionality in a separate method named collectParameters() - it collects the signature and values, the return value is used to determine if the signature has null parameter types.

Ovidiu Maxiniuc wrote:

Dănuț Filimon wrote:

Ovidiu, do you have any small example that will reach createImport()? I've tried to create one similar to one of the patterns used in the application (before buffer, dynamic query) but I did not manage to reproduce it.

Using a built-in 4GL function which FWD implemented in a different package should do the trick.
Creating a dynamic query for the FILL or SAVE-CHANGES methods in a Dataset with the DataSource from a different (permanent) database should add the imports needed for that related DMO.

I tried to do an example with FILL and I finally managed to get to debug the collectImports()! Now I noted that when we create a dynamic query in this type of setting we have dmos for the tables (fwd, _meta, _temp) and all of them used the correct package, so the problem should be related to dynamically created tables or before buffers that do not have a dmo. I am getting close to creating a proper example for this.

I got blocked again as I am not able to reproduce the "null.*" import, the latest example I managed to create is:

DEFINE TEMP-TABLE tt2 NO-UNDO BEFORE-TABLE btt2
    FIELD f1 AS CHARACTER
    FIELD f2 AS CHARACTER
    FIELD f3 AS CHARACTER
    FIELD f4 AS CHARACTER
    FIELD f5 AS CHARACTER INDEX idx1 IS PRIMARY UNIQUE f1 f2 f3.

DEFINE VARIABLE h-tt AS HANDLE NO-UNDO.
DEFINE VARIABLE htt AS HANDLE NO-UNDO.
CREATE TEMP-TABLE h-tt.
h-tt:CREATE-LIKE('tt2').
h-tt:TEMP-TABLE-PREPARE ('tt1').
htt = h-tt:DEFAULT-BUFFER-HANDLE.

DEFINE VARIABLE ds1 AS HANDLE NO-UNDO.
DEFINE VARIABLE srctt2 AS HANDLE NO-UNDO.
DEFINE VARIABLE htt2 AS HANDLE NO-UNDO.
CREATE BUFFER htt2 FOR TABLE "tt2".

DEFINE VARIABLE qh AS HANDLE NO-UNDO.
CREATE QUERY qh.
qh:ADD-BUFFER(htt2).

CREATE DATASET ds1.
ds1:ADD-BUFFER(htt).

h-tt:TRACKING-CHANGES = TRUE.
htt:BUFFER-CREATE().
htt:BUFFER-FIELD("f1"):BUFFER-VALUE(0) = "A6".
htt:BUFFER-FIELD("f2"):BUFFER-VALUE(0) = "B6".
htt:BUFFER-FIELD("f3"):BUFFER-VALUE(0) = "C6".
htt:BUFFER-FIELD("f4"):BUFFER-VALUE(0) = "D6".
htt:BUFFER-FIELD("f5"):BUFFER-VALUE(0) = "E6".
h-tt:TRACKING-CHANGES = FALSE.

CREATE DATA-SOURCE srctt2.
srctt2:QUERY = qh.

htt:ATTACH-DATA-SOURCE(srctt2).
DO TRANSACTION ON ERROR UNDO, LEAVE:
    qh:SET-BUFFERS(ds1:GET-BUFFER-HANDLE(1):BEFORE-BUFFER).
    qh:QUERY-PREPARE("FOR EACH " + ds1:GET-BUFFER-HANDLE(1):BEFORE-BUFFER:TABLE).
    qh:QUERY-OPEN().
    qh:GET-FIRST().
    DO WHILE NOT qh:QUERY-OFF-END ON ERROR UNDO, LEAVE:
        ds1:GET-BUFFER-HANDLE(1):BEFORE-BUFFER:SAVE-ROW-CHANGES().
        qh:GET-NEXT().
    END.
    qh:QUERY-CLOSE().        
END.
htt:DETACH-DATA-SOURCE().

DELETE OBJECT srctt2.

Compared to the tree dump when collectImportPaths() is called (from #9032-10), the dump looks like this:

compilation unit [COMPILE_UNIT]:25769803777 @0:0
   com.goldencode.p2j.util.* [KW_IMPORT]:25769803779 @0:0
   com.goldencode.p2j.util.BlockManager.* [STATIC_IMPORT]:25769803780 @0:0
   DynGenQuery [KW_CLASS]:25769803781 @0:0
         (access=public, pkgname=com.goldencode.p2j.persist.dynquery)
       [CS_INSTANCE_VARS]:25769803785 @0:0
         = [ASSIGN]:25769803850 @0:0
            AdaptiveQuery [REFERENCE_DEF]:25769803851 @0:0
                  (name=query0, final=false)
            AdaptiveQuery [CONSTRUCTOR]:25769803852 @0:0
                  (runtime-class=class com.goldencode.p2j.persist.AdaptiveQuery, runtime-constructor=public com.goldencode.p2j.persist.AdaptiveQuery())
       [CS_INSTANCE_METHODS]:25769803788 @0:0
         execute [METHOD_DEF]:25769803790 @0:0
             [BLOCK]:25769803791 @0:0
               initialize [METHOD_CALL]:25769803855 @0:0
                     (runtime-class=class com.goldencode.p2j.persist.AdaptiveQuery, runtime-method=public com.goldencode.p2j.persist.PreselectQuery com.goldencode.p2j.persist.PreselectQuery.initialize(com.goldencode.p2j.persist.DataModelObject,java.lang.String,java.util.function.Supplier,java.lang.String,java.lang.String))
                  query0 [REFERENCE]:25769803856 @0:0
                  bitt1 [REFERENCE]:25769803857 @0:0
                  String [CAST]:25769803858 @0:0
                        (runtime-class=class java.lang.String)
                      [NULL_LITERAL]:25769803859 @0:0
                           (runtime-value=null)
                   [NULL_LITERAL]:25769803860 @0:0
                        (runtime-value=null)
                  bitt1._rowState asc [STRING]:25769803861 @0:0
                        (runtime-value=bitt1._rowState asc)
                  WHOLE-INDEX,rowState [STRING]:25769803862 @0:0
                        (runtime-value=WHOLE-INDEX,rowState)
   com.goldencode.p2j.persist.* [KW_IMPORT]:25769803817 @0:0
   com.goldencode.p2j.persist.dynamic._temp.* [KW_IMPORT]:25769803818 @0:0
   com.goldencode.p2j.persist.lock.* [KW_IMPORT]:25769803863 @0:0

Created 9032b, this branch will contain conversion changes related to FindQuery silent operations with the purpose of avoiding lambdas. I've made a basic implementation and right now I am working on reducing the rules used to a much simpler form, should be available tomorrow.

Ovidiu: Please review.

Review of 9032a/15367-9.

I think the code is good. There are some issue I spotted:

• RuntimeJastInterpreter.java:
  • line 917: the line can be dropped: the assert is not be meant for parameter validation;
  • line 928: hasNullParameterType is correctly evaluated only if signature != null. Otherwise false is always returned, even there are some nulls in values;
  • in places where the 'enhanced for' was replaced by indexed variant and there is a single occurrence of the iterated element (3 locations), it can be inlined;
• XmlImport.java:
  • line 2376: since you initialized decimals when it was declared, this line can be dropped, or better, replaced with a comment saying that in case of NFE, the value of decimals remains 0. I wonder whether Java ♨️ is smart enough to remove it at compile time;
• ControlFlowOps.java:
  • line 9837: shouldn't the condition be if (p == null || p instanceof unknown)?

Ovidiu Maxiniuc wrote:

Review of 9032a/15367-9.

I think the code is good. There are some issue I spotted:

• ControlFlowOps.java:
  • line 9837: shouldn't the condition be if (p == null || p instanceof unknown)?

Not at all, this change only targets converted calls such as:

RUN_CALL_SITE_6.clone().run(new unknown());

Even if there are cases where values are passed, those are BaseDataType, handles which by default are initialized with unknown so in the first place I don't think it is possible to have a null parameter (I checked hundreds RUN_CALL_SITE calls in a customer project and found no such occurrence, but please do correct me if I am wrong). There are also other types that can be passed such as DatasetParameter and so on.

Fixed in 9032a/15370.

Tested hotel_gui conversion and found the the following problem:

    [javac] Compiling 117 source files to /home/ddf/gcd/hotel_gui/build/classes
    [javac] Compiling 61 source files to /home/ddf/gcd/hotel_gui/build/classes
    [javac] /home/ddf/gcd/hotel_gui/src/com/goldencode/hotel/h2_tester/QueriesTester.java:458: error: ';' expected
    [javac]          }).silentFirst()for (i.assign(1); _isLessThanOrEqual(i, 1000); i.increment())
    [javac]                          ^
    [javac] /home/ddf/gcd/hotel_gui/src/com/goldencode/hotel/h2_tester/QueriesTester.java:488: error: ';' expected
    [javac]          }).silentFirst()for (i.assign(1); _isLessThanOrEqual(i, 1000); i.increment())
    [javac]                          ^
    [javac] /home/ddf/gcd/hotel_gui/src/com/goldencode/hotel/h2_tester/QueriesTester.java:518: error: ';' expected
    [javac]          }).silentFirst()for (i.assign(1); _isLessThanOrEqual(i, 1000); i.increment())
    [javac]                          ^
    [javac] 3 errors

    //------------------------------------------------------
    iRowsAffected = 0.
    ETIME(YES).
    DO i = 1 TO 1000 :
        FIND FIRST tt2 WHERE tt2.f1 = (i * 9) MOD 900 NO-ERROR.
    END.
    tm = ETIME(NO).

    RUN createField IN hTools1 (INPUT "find", INPUT "FIND-FIRST-WHERE-FIELD-=", INPUT iRowsAffected, INPUT tm, INPUT-OUTPUT TABLE t-field).
    //------------------------------------------------------
    iRowsAffected = 0.
    ETIME(YES).
    DO i = 1 TO 1000 :
        FIND FIRST tt2 WHERE tt2.fId > i * 9.
    END.
    tm = ETIME(NO).

    RUN createField IN hTools1 (INPUT "find", INPUT "FIND-FIRST-WHERE-INDEX->", INPUT iRowsAffected, INPUT tm, INPUT-OUTPUT TABLE t-field).
    //------------------------------------------------------

         // ------------------------------------------------------
         iRowsAffected.assign(0);
         date.elapsed(true);

         loopLabel15:
         new FindQuery(tt2, "tt2.f1 = ?", null, "tt2.fid asc", new Object[]
         {
            modulo(multiply(i, 9), 900)
         }).silentFirst()for (i.assign(1); _isLessThanOrEqual(i, 1000); i.increment())
;

         tm.assign(date.elapsed(false));
         RUN_CALL_SITE_15.clone().setInHandle(hTools1).run(new character("find"), new character("FIND-FIRST-WHERE-FIELD-="), iRowsAffected, tm, new TableParameter(tField));         

         // ------------------------------------------------------
         iRowsAffected.assign(0);
         date.elapsed(true);

         loopLabel16:
         for (i.assign(1); _isLessThanOrEqual(i, 1000); i.increment())
         {
            new FindQuery(tt2, "tt2.fid > ?", null, "tt2.fid asc", new Object[]
            {
               multiply(i, 9)
            }).first();
         }

         tm.assign(date.elapsed(false));
         RUN_CALL_SITE_16.clone().setInHandle(hTools1).run(new character("find"), new character("FIND-FIRST-WHERE-INDEX->"), iRowsAffected, tm, new TableParameter(tField));         

         // ------------------------------------------------------

Currently investigating.

A peer ast is created correctly for:

[java] block [BLOCK]:317827580127 @0:0
[java]       (peerid-byrule=convert/language_statements.rules:281, byrule=convert/language_statements.rules:281, peerid=21474836784)

[java] block [BLOCK]:317827580123 @0:0
[java]       (peerid-byrule=convert/control_flow.rules:2532, byrule=convert/control_flow.rules:2532, peerid=21474836783)

Dănuț Filimon wrote:

A peer ast is created correctly for:
[...] but with 9032b it is created for:
[...]

If I don't use NO-ERROR, the FindQuery is generated correctly on a similar block when using 9032b.

I tested the same customer scenario from #9032-35 (100 runs) using 9032a and monitored the process using VisualVM. There's no memory leak and the heap size was slowly decreasing (2h 45 min server uptime, the scenario took ~40 minutes).

1. no brackets used for the body of a block
   

      // 4GL code
      METHOD PUBLIC VOID create1():
          FIND tt1 NO-ERROR.
      END METHOD.
   
      // 9032b
      @LegacySignature(type = Type.METHOD, name = "create1")
      public void create1()
      {
         internalProcedure(FindQueryUnique1noerror.class, this, "create1", new Block((Body) () -> new FindQuery(tt1, (String) null, null, "tt1.recid asc").silentUnique()));
      }
   
      // 4GL code, same method but without NO-ERROR
      METHOD PUBLIC VOID create1():
          FIND tt1.
      END METHOD.
   
      // 9032b
      @LegacySignature(type = Type.METHOD, name = "create1")
      public void create1()
      {
         internalProcedure(FindQueryUnique1.class, this, "create1", new Block((Body) () -> 
         {
            new FindQuery(tt1, (String) null, null, "tt1.recid asc").unique();
         }));
      }
   

   If there is any other operation that is in the same procedure/block with the FindQuery, it will convert correctly.
2. no brackets used for IF ... THEN DO
   

      // 4GL code
      METHOD PUBLIC VOID create49():
          IF i EQ TRUE THEN DO:
              FIND tt1 NO-ERROR.
          END.
      END METHOD.
   
      // 9032b
      @LegacySignature(type = Type.METHOD, name = "create49")
      public void create49()
      {
         internalProcedure(FindQueryUnique1noerror.class, this, "create49", new Block((Body) () -> 
         {
            if (_isEqual(i, true))
   new FindQuery(tt1, (String) null, null, "tt1.recid asc").silentUnique()      }));
      }
   

   If there is any other operation that is in the if statement with the FindQuery, it will convert correctly.
3. #9032-41

There are more cases where the FindQuery does not convert correctly, but all of them have the missing brackets in common. My implementation idea was to replace the methodtxt = "first" with "silentFirst" when the statement node would have the "no-silent-lambda" annotation and my intention was to let it generate like a normal first() call. I am currently revising the implementation because I can create a very small test case and comparing the queries with and without NO-ERROR to maybe identify where it is branching off.

The problem with point 1 from #9032-45 is that when the rules reach include/common-progress.rules:11795 it will call:

         <!-- now we can create the node to be returned -->
         <rule>
            retNode = tw.graft("block_lambda_def", blockNode, destNodeId,
                               "parent",   constructor,
                               "finiType", finiType)
         </rule>

                    <ast col="0"  line="0" text="" type="LPARENS">
                      <annotation datatype="java.lang.String" key="byrule" value="include/common-progress.rules:11795"/>
                    </ast>
                    <ast col="0"  line="0" text="block" type="BLOCK">
                      <annotation datatype="java.lang.String" key="byrule" value="include/common-progress.rules:11795"/>
                      <annotation datatype="java.lang.String" key="peerid-byrule" value="include/common-progress.rules:11795"/>
                      ...

I thought that the METHOD_CALL AST overrides the BLOCK AST in database_access.rule:1267 but I checked the created ast/parent and have the following AST after the call:

block [BLOCK]:274877907050 @0:0
     [java]       (byrule=include/common-progress.rules:11795, peerid-byrule=include/common-progress.rules:11795, peerid=90194313237)
     [java]    silentUnique [METHOD_CALL]:274877907056 @0:0
     [java]          (byrule=convert/database_access.rules:1267, peerid-byrule=convert/database_access.rules:1267, peerid=90194313239)

Committed 9032a/15380 9032b/15380. Fixed deleted BLOCK node for FIND ... NO-ERROR when there is no silent lambda.

Dănuț Filimon wrote:

Now I have another conversion issue where an AdaptiveQuery ends up using the silent methods so I will also begin my investigation on this one.

This issue is actually related to AdaptiveFind, the methods are replaced correctly but are not implemented.

Alexandru, what do you think about removing the silent lambda for the AdaptiveFind query? I'll only have to implement the methods for this. The second option is opt AdaptiveFind from using silent methods and continue to use the silent lambda (still need to look into how to do this, but it will take more time).

It is right to extend the optimization to the AdaptiveFind as well.

Committed 9032b/15381. Added silentFirst() and silentNext() methods for AdaptiveFind.

My conversion tests pass now and the project compiles with no problems. I am switching to creating using tests and comparing the execution results with/without the changes. This will take a while as I need to make sure I cover most of the scenarios, I also think we have tests for the FindQuery in the testcases project so I'll also take a look there.

Alexandru: Please review.

The changes are quite extensive and I will need a bit more time to get through them. I will re-schedule the review attempt tomorrow morning.

I found issues while running performance tests. It seems that FIND NEXT, FIND PREV are not actually executing. Currently investigating...

Dănuț Filimon wrote:

I found issues while running performance tests. It seems that FIND NEXT, FIND PREV are not actually executing. Currently investigating...

There is a scenario where a QueryOffEndException is thrown when calling silent methods which is not caught at the method level and causes the program to stop executing.

Alexandru Lungu wrote:

I just updated the branch but didn't had the chance to properly review it. Please consider fixing the found problem and I will resume the review after.

I am already testing the fix (just catching the QueryOffEndException in the silent method) but I am not sure if there is this error should be processed or left as it is.

• Without 9032b (measurements are in ms):

  Test	Run 1 (Cold)	Run 2	Run 3	Run 4	Run 5	Average
  tt 10 FIND FIRST	2	1	0	1	0	0.8
  tt 100 FIND FIRST	10	2	1	1	1	3
  tt 1000 FIND FIRST	108	32	28	28	29	45
  tt 10000 FIND FIRST	2450	2735	2693	2711	2683	2654.4
  tt 10 FIND LAST	8	7	10	10	7	8.4
  tt 100 FIND LAST	57	51	51	52	53	52.8
  tt 1000 FIND LAST	500	513	528	503	530	514.8
  tt 10000 FIND LAST	4864	5245	5315	5140	5472	5207.2
  tt 10 FIND	9	6	7	7	11	8
  tt 100 FIND	55	54	53	51	54	53.4
  tt 1000 FIND	549	515	535	511	564	534.8
  tt 10000 FIND	5337	5347	5220	5100	5573	5315.4
  pt 10 FIND FIRST	5	15	14	14	15	12.6
  pt 100 FIND FIRST	21	129	132	132	135	109.8
  pt 1000 FIND FIRST	187	1319	1346	1321	1338	1102.2
  pt 10000 FIND FIRST	5771	12792	13129	13279	13303	11654.8
  pt 10 FIND LAST	11	14	13	14	15	13.4
  pt 100 FIND LAST	105	128	126	131	134	124.8
  pt 1000 FIND LAST	968	1277	1254	1319	1354	1234.4
  pt 10000 FIND LAST	5384	12814	12545	13234	13232	11441.8
  pt 10 FIND	15	13	14	15	14	14.2
  pt 100 FIND	130	124	126	130	129	127.8
  pt 1000 FIND	1257	1248	1265	1308	1287	1273
  pt 10000 FIND	12551	12601	12606	13205	12703	12733.2

The cold run has the best results out of all the runs for 10k find queries especially on the FIND FIRST/LAST of a persistent table with the cold run showing ~5000ms while the rest of the runs exceed 10000ms.

• With 9032b (measurements are in ms):

  Test	Run 1 (Cold)	Run 2	Run 3	Run 4	Run 5	Average
  tt 10 FIND FIRST	1	1	0	0	0	0.4
  tt 100 FIND FIRST	9	1	0	1	1	2.4
  tt 1000 FIND FIRST	84	27	29	27	29	39.2
  tt 10000 FIND FIRST	2573	2625	2625	2633	2676	2626.4
  tt 10 FIND LAST	9	7	6	7	7	7.2
  tt 100 FIND LAST	55	50	53	51	51	52
  tt 1000 FIND LAST	522	517	513	498	529	515.8
  tt 10000 FIND LAST	5181	5196	5131	5128	5352	5197.6
  tt 10 FIND	8	7	6	7	9	7.4
  tt 100 FIND	54	52	49	50	52	51.4
  tt 1000 FIND	520	506	505	508	512	510.4
  tt 10000 FIND	5135	5036	5034	5152	5182	5107.8
  pt 10 FIND FIRST	17	14	15	15	14	15.0
  pt 100 FIND FIRST	140	134	132	135	134	135
  pt 1000 FIND FIRST	1309	1300	1306	1333	1332	1316
  pt 10000 FIND FIRST	12990	13190	12992	13437	13150	13151.8
  pt 10 FIND LAST	14	14	14	14	13	13.8
  pt 100 FIND LAST	132	132	138	133	127	132.4
  pt 1000 FIND LAST	1259	1356	1353	1340	1272	1316
  pt 10000 FIND LAST	12664	12991	13503	13479	12689	13065.2
  pt 10 FIND	14	13	14	15	14	14
  pt 100 FIND	126	126	150	132	130	132.8
  pt 1000 FIND	1269	1272	1354	1365	1302	1312.4
  pt 10000 FIND	12620	12647	13418	13543	12940	13033.6

The same can't be said when using 9032b.

Dănuț Filimon wrote:

I am already testing the fix (just catching the QueryOffEndException in the silent method) but I am not sure if there is this error should be processed or left as it is.

Just catching the QueryOffEndException is not the fix! The root cause is that the FindQuery constructor relies on the following statement:

setLenientOffEnd(getBuffer().txHelper.errHlp.isSilent());

I also noted during debugging that there are two lenientOffEnd flags (one in RandomAccessQuery and one in AbstractQuery) and after discussing with Eduard/Alexandru, this should not be the case and the one from RandomAccessQuery might have been left because of a wrong rebase/merge.

Review on 9032b:

• In AdaptiveFind, isn't better to do delegate the lenientOffEnd stuff to silent* method entirely. I don't like the fact that we are working with lenient off end both in silent* and non-silent* methods. Can we simply stick to how this is done in RandomAccessQuery? (a.k.a set the lenient-off end directly in the silent* methods?)
• Please cache the ErrorHelper as a class member. That is, set-it up in the initialize method, to avoid retrieving at each operation.
• Rework ErrorManager.silentWorker to use processLegacyErrorException (which I think can be made static in ErrorManager anyway).

Also, can you share here the test you are doing for profiling?

So you are testing the case when records are found? This is only checking the lambda removal. I think you should have some tests where a "not found" exception is actually thrown and see that lenientOffEnd is faster than exception throwing.

TBD: the times you are seeing are severely spoiled by the actual database look-up that is done in order to find the record. Also, some queries are subject to fast-find cache. I suggest working with an empty table and avoid the build-up of ffcache (doing 10/100/1000 finds). After 100 finds, the first 100 finds from the total of 1000 will be cached already.

This looks right! I am OK with the changes.

• without 9032b See resultsSee results

  Test	Run 1 (Cold)	Run 2	Run 3
  tt 10 FIND CURRENT	10	0	0
  tt 100 FIND CURRENT	4	1	1
  tt 1000 FIND CURRENT	27	2	1
  tt 10000 FIND CURRENT	57	17	8
  tt 100000 FIND CURRENT	120	89	65
  tt 250000 FIND CURRENT	180	154	164
  tt 10 FIND FIRST	19	1	1
  tt 100 FIND FIRST	7	0	1
  tt 1000 FIND FIRST	43	2	2
  tt 10000 FIND FIRST	82	17	14
  tt 100000 FIND FIRST	272	143	135
  tt 250000 FIND FIRST	379	316	337
  tt 10 FIND NEXT	11	0	1
  tt 100 FIND NEXT	0	0	0
  tt 1000 FIND NEXT	5	1	0
  tt 10000 FIND NEXT	15	3	3
  tt 100000 FIND NEXT	46	23	20
  tt 250000 FIND NEXT	52	58	56
  tt 10 FIND PREV	1	1	1
  tt 100 FIND PREV	0	0	0
  tt 1000 FIND PREV	3	0	0
  tt 10000 FIND PREV	10	3	2
  tt 100000 FIND PREV	54	29	23
  tt 250000 FIND PREV	60	63	56
  tt 10 FIND LAST	2	0	0
  tt 100 FIND LAST	1	0	0
  tt 1000 FIND LAST	9	2	1
  tt 10000 FIND LAST	38	16	16
  tt 100000 FIND LAST	193	154	143
  tt 250000 FIND LAST	367	301	358
  tt 10 FIND	1	1	1
  tt 100 FIND	1	0	1
  tt 1000 FIND	5	2	1
  tt 10000 FIND	28	15	14
  tt 100000 FIND	195	140	129
  tt 250000 FIND	330	307	342
  pt 10 FIND CURRENT	1	0	0
  pt 100 FIND CURRENT	0	0	1
  pt 1000 FIND CURRENT	5	1	0
  pt 10000 FIND CURRENT	25	8	6
  pt 100000 FIND CURRENT	117	57	63
  pt 250000 FIND CURRENT	149	134	136
  pt 10 FIND FIRST	5	2	1
  pt 100 FIND FIRST	30	6	6
  pt 1000 FIND FIRST	201	59	56
  pt 10000 FIND FIRST	952	557	555
  pt 100000 FIND FIRST	5402	5450	5750
  pt 250000 FIND FIRST	13082	13693	13679
  pt 10 FIND NEXT	2	1	1
  pt 100 FIND NEXT	0	0	0
  pt 1000 FIND NEXT	3	1	0
  pt 10000 FIND NEXT	19	3	2
  pt 100000 FIND NEXT	82	21	21
  pt 250000 FIND NEXT	55	60	43
  pt 10 FIND PREV	1	1	1
  pt 100 FIND PREV	1	0	0
  pt 1000 FIND PREV	0	0	0
  pt 10000 FIND PREV	3	3	3
  pt 100000 FIND PREV	24	19	20
  pt 250000 FIND PREV	54	56	46
  pt 10 FIND LAST	2	1	1
  pt 100 FIND LAST	11	9	6
  pt 1000 FIND LAST	90	62	54
  pt 10000 FIND LAST	672	552	547
  pt 100000 FIND LAST	6869	5583	5540
  pt 250000 FIND LAST	14561	14063	13979
  pt 10 FIND	2	1	1
  pt 100 FIND	5	4	4
  pt 1000 FIND	40	41	40
  pt 10000 FIND	415	380	371
  pt 100000 FIND	3851	4258	3855
  pt 250000 FIND	9244	9427	9779

• with 9032b See resultsSee results

  Test	Run 1 (Cold)	Run 2	Run 3
  tt 10 FIND CURRENT	9	0	0
  tt 100 FIND CURRENT	3	0	1
  tt 1000 FIND CURRENT	23	1	1
  tt 10000 FIND CURRENT	41	9	7
  tt 100000 FIND CURRENT	96	69	67
  tt 250000 FIND CURRENT	148	151	151
  tt 10 FIND FIRST	18	1	0
  tt 100 FIND FIRST	5	0	0
  tt 1000 FIND FIRST	35	1	2
  tt 10000 FIND FIRST	73	13	24
  tt 100000 FIND FIRST	247	120	118
  tt 250000 FIND FIRST	304	295	289
  tt 10 FIND NEXT	11	2	1
  tt 100 FIND NEXT	1	0	0
  tt 1000 FIND NEXT	6	0	1
  tt 10000 FIND NEXT	20	2	4
  tt 100000 FIND NEXT	62	27	32
  tt 250000 FIND NEXT	73	81	81
  tt 10 FIND PREV	1	1	1
  tt 100 FIND PREV	0	0	0
  tt 1000 FIND PREV	2	0	0
  tt 10000 FIND PREV	9	4	3
  tt 100000 FIND PREV	55	33	36
  tt 250000 FIND PREV	76	84	86
  tt 10 FIND LAST	1	1	1
  tt 100 FIND LAST	2	0	1
  tt 1000 FIND LAST	7	1	2
  tt 10000 FIND LAST	28	15	13
  tt 100000 FIND LAST	158	142	139
  tt 250000 FIND LAST	331	342	337
  tt 10 FIND	2	1	1
  tt 100 FIND	1	0	0
  tt 1000 FIND	10	2	2
  tt 10000 FIND	24	12	12
  tt 100000 FIND	151	123	121
  tt 250000 FIND	315	326	306
  pt 10 FIND CURRENT	0	0	0
  pt 100 FIND CURRENT	1	1	0
  pt 1000 FIND CURRENT	6	0	0
  pt 10000 FIND CURRENT	23	4	6
  pt 100000 FIND CURRENT	86	48	50
  pt 250000 FIND CURRENT	124	132	127
  pt 10 FIND FIRST	5	1	1
  pt 100 FIND FIRST	28	7	8
  pt 1000 FIND FIRST	164	58	77
  pt 10000 FIND FIRST	696	556	569
  pt 100000 FIND FIRST	5542	5343	5432
  pt 250000 FIND FIRST	13096	13338	14168
  pt 10 FIND NEXT	1	1	0
  pt 100 FIND NEXT	1	0	0
  pt 1000 FIND NEXT	2	0	0
  pt 10000 FIND NEXT	10	3	3
  pt 100000 FIND NEXT	69	29	30
  pt 250000 FIND NEXT	71	75	75
  pt 10 FIND PREV	1	1	1
  pt 100 FIND PREV	0	0	0
  pt 1000 FIND PREV	2	1	1
  pt 10000 FIND PREV	4	5	3
  pt 100000 FIND PREV	34	32	31
  pt 250000 FIND PREV	66	73	74
  pt 10 FIND LAST	2	2	2
  pt 100 FIND LAST	9	6	6
  pt 1000 FIND LAST	69	59	56
  pt 10000 FIND LAST	608	547	549
  pt 100000 FIND LAST	5359	5643	5457
  pt 250000 FIND LAST	13955	13770	14267
  pt 10 FIND	1	1	1
  pt 100 FIND	6	4	4
  pt 1000 FIND	39	37	40
  pt 10000 FIND	379	420	373
  pt 100000 FIND	3695	3780	3746
  pt 250000 FIND	9321	9522	9432

• tt 250000 FIND CURRENT: With 9032b, the cold results matches the warm run results when not using 9032b;
• tt 100000 FIND FIRST and tt 250000 FIND FIRST improved;
• tt 100000 FIND NEXT and tt 250000 FIND NEXT are slower with 9032b;
• tt 100000 FIND PREV and tt 250000 FIND PREV are slower with 9032b;
• tt 100000 FIND LAST cold run improved, but not the same can be said about tt 250000 FIND LAST
• tt 100000 FIND improved;
• pt 100000 FIND CURRENT and pt 250000 FIND CURRENT improved;
• pt 250000 FIND FIRST one war run shows a worse result;
• pt 100000 FIND LAST and pt 250000 FIND LAST cold run improvements;
• pt 100000 FIND cold run improvement, but slower warm runs;

addExternalBuffers in FindQuery need to return FindQuery, as a FIND ... WHERE CAN-FIND() will emit an addExternalBuffers and the call chain will be like new FindQuery().addExternalBuffers().silentFirst() - this fails javac compile, as RandomAccessQuery.addExternalBuffers() returns P2JQuery.

Constantin Asofiei wrote:

addExternalBuffers in FindQuery need to return FindQuery, as a FIND ... WHERE CAN-FIND() will emit an addExternalBuffers and the call chain will be like new FindQuery().addExternalBuffers().silentFirst() - this fails javac compile, as RandomAccessQuery.addExternalBuffers() returns P2JQuery.

Thank you! I've fixed this in 9032b/15384, the code compiles now after a small test. I've also tried to reproduce this with AdaptiveFind, but the same where clause is actually used in the initialize() method so the same does not apply to the AdaptiveFindQuery.

Rebased 9032b to trunk/15393, the branch is now at revision 15399.

Good news, I tested a hotel_gui and a customer application conversion and both were successful.

The #8363 app converted and compiled OK, and performance run showed a small improvement. I need to reconvert ETF and run with it.

Dănuț Filimon wrote:

Good news, I tested a hotel_gui and a customer application conversion and both were successful.

Smoke tests went well, but when I did a test on the customer unit tests it went into an infinite loop after 2 modules, while testing without any changes (old conversion) also showed an infinite loop in a different module. I am not sure if this customer has a spreadsheet with the results as the one from #8784-1.

There are conversion regressions for this construct:

DO PRESELECT EACH tt1 WHERE tt1.f1 > 0::
  FIND NEXT tt1 NO-ERROR.
END.

This emits a silentNext() which is not defined for PreselectQuery.

The point: a FIND NEXT insinde a {DO|PRESELECT} PRESELECT block is not a FindQuery. We either move the silentNext/First/etc from FindQuery up to AbstractQuery, or change conversion rules to exclude these cases. I'd go with the first proposal, change runtime.

Constantin Asofiei wrote:

There are conversion regressions for this construct:
[...]

This emits a silentNext() which is not defined for PreselectQuery.

The point: a FIND NEXT insinde a {DO|PRESELECT} PRESELECT block is not a FindQuery. We either move the silentNext/First/etc from FindQuery up to AbstractQuery, or change conversion rules to exclude these cases. I'd go with the first proposal, change runtime.

This is a problem... In RandomAccessQuery I defined silentFirst(), silentFirst(Object[]), silentFirst(LockType), silentFirst(Object[], LockType), the first three calling the last one. If we move those methods, we will not have a match for the PrelesectQuery as there is no first method that takes two parameters.

Dănuț Filimon wrote:

This is a problem... In RandomAccessQuery I defined silentFirst(), silentFirst(Object[]), silentFirst(LockType), silentFirst(Object[], LockType), the first three calling the last one. If we move those methods, we will not have a match for the PrelesectQuery as there is no first method that takes two parameters.

Actually this can work, but we will have to remove the silentFirst() and silentFirst(LockType) methods from RandomAccessQuery and AdaptiveFind, while silentFirst(Object[]) and silentFirst(Object[], LockType) will still be needed and not moved to AbstractQuery. Another nice change would be moving the errorHelper property to the AbstractQuery and using a method to initialize and use it (this will help avoid any future problems where errorHelper would be needed in the AbstractQuery and not be removed from AdaptiveFindQuery/RandomAccessQuery by mistake).

I tested 9032b with customer unit tests and there were no regressions.

Just to be sure, 9032b needs to be conversion tested, and not 9032a, right?

Radu Apetrii wrote:

Just to be sure, 9032b needs to be conversion tested, and not 9032a, right?

Yes.

Conversion was OK with 9032b for the project.

I managed to convert the regression xfer project, but had problems with the compilation (unrelated to the changes) which I reported in #7825-40.

Conversion went fine with 9032b for the project that I tested.

ETF testing also passed.

Created 9032c from trunk/15407. I'll use this branch to reduce the number of ErrorConditionException that are being thrown, especially for FIND .. NO-ERROR. I have a basic implementation down, but I want make sure that there are no issues (a customer test - 100 runs scenario) before adding the javadoc and committing it.

Note that only a percentage of those are using silent mode (tested without any changes from 9032a or 9032b).

Committed 9032c/15408. Suppress 138 and 565 4GL exceptions in silent mode, those exceptions will be available through the WorkArea of the ErrorManager and checked for the force flag (similar to what it is done in the catch block of the silent worker).

As I am unable to finish building the regression xfer project due to #9032-88. What are the remaining tests that should be done for 9032b?

9032b can be merged to trunk now

I will be merging it soon, ~5-10 min.

Dănuț Filimon wrote:

I will be merging it soon, ~5-10 min.

Nevermind, it will take a bit more. I get

brz: ERROR: Key b'osresource-20240821155930-wbmwcxlkbxi24t8y-1' is already present in map

Dănuț Filimon wrote:

Dănuț Filimon wrote:

I will be merging it soon, ~5-10 min.

Nevermind, it will take a bit more. I get
[...] when I try to rebase.

Do this from devsrv01.

Constantin Asofiei wrote:

Do this from devsrv01.

Do I have to checkout the branches there and do it?

Dănuț Filimon wrote:

Constantin Asofiei wrote:

Do this from devsrv01.

Do I have to checkout the branches there and do it?

It still does not work. I also did #7143-1175.

Where did you checkout trunk? You need to have a checkout of trunk and a checkout of 9032b, and after that rebase on the trunk checkout.

Constantin Asofiei wrote:

Where did you checkout trunk? You need to have a checkout of trunk and a checkout of 9032b, and after that rebase on the trunk checkout.

Radu made things clear for me and I am now checking out both 9032b and trunk on devsrc01 /home/ddf/branches/.

Branch 9032b has been merged intro trunk/15411 and archived.

Dănuț Filimon wrote:

I am putting this in WIP for 9032c at 50%, I still want to investigate other common errors as the ones mentioned in #9032-93 where the same change can be done for exceptions like 138 and 565.

The rest of the implementation for 9032c right now is influenced by the changes from 9032b which made it into trunk/15411 so I will also have to address the changes after a rebase.

Rebased 9032c to trunk/15411, the branch is now at revision 15412.

Committed 9032c/15413. Moved the silent mode method finally block into a separate method.

Committed 9032c/15414. 9032c is now compatible with the 9032b implementation that was merged in trunk/15411.

Constantin, it seems that there's a problem with the FindQuery constructor. There's the possibility of it throwing an exception which is no longer accounted in the silent operation. Lorian reported this to me after he rebased 9095a and retested his changes.

Dănuț Filimon wrote:

Constantin, it seems that there's a problem with the FindQuery constructor. There's the possibility of it throwing an exception which is no longer accounted in the silent operation. Lorian reported this to me after he rebased 9095a and retested his changes.

What is the stacktrace?

There's no stack trace, thrown errors will get displayed on the screen when they should not (because they were silent). I can provide the example Lorian gave me.

Dănuț Filimon wrote:

There's no stack trace, thrown errors will get displayed on the screen when they should not (because they were silent). I can provide the example Lorian gave me.

Well, find where the error is thrown and capture it in the debugger. Plus a standalone test will help.

Note that this is only visible with 9095a changes, the patch:

=== modified file 'src/com/goldencode/p2j/persist/FQLPreprocessor.java'
--- old/src/com/goldencode/p2j/persist/FQLPreprocessor.java    2024-07-03 14:06:51 +0000
+++ new/src/com/goldencode/p2j/persist/FQLPreprocessor.java    2024-09-02 10:49:58 +0000
@@ -405,6 +405,7 @@
 import com.goldencode.p2j.persist.dialect.*;
 import com.goldencode.p2j.persist.hql.*;
 import com.goldencode.p2j.persist.pl.*;
+import com.goldencode.p2j.util.ErrorManager;
 import com.goldencode.p2j.util.*;
 import com.goldencode.p2j.util.logging.CentralLogger;

@@ -5180,6 +5181,12 @@
       Aast alias = parent != null && parent.getType() == ALIAS ? parent : null;
       RecordBuffer rbuff = lookupBuffer(alias);
       String denormalizedProperty = TableMapper.getDenormalizedProperty(rbuff, ast.getText(), extentIndex);
+      
+      if (denormalizedProperty == null)
+      {
+         ErrorManager.recordOrThrowError(26, String.valueOf(extentIndex), "");
+      }
+      
       ast.setText(denormalizedProperty);
       ast.removeChildren();
       ast.putAnnotation("inlined", Boolean.TRUE);

start.p:

create test_table.
test_table.f1[1] = 10.
release test_table.

def var iPos as int.
def var cAlphabet       as char init "ABCDEFGHIJKLMNOPQRSTUVWXYZ" no-undo.
def var cChar as char.

cChar = '1'.
iPos = index(cAlphabet, cChar, 1).

def var i as int.
function test int:
    find first test_table where f1[iPos] ne 0 no-error.
    if (error-status:num-messages > 0) then do:
        message error-status:error error-status:num-messages.
    end.
    message "test method called".
    return 1.
end.

find first test_table where test () eq 1 no-lock no-error.
message "executed". 

fwd.df

ADD TABLE "test_table" 
  AREA "Schema Area" 
  DUMP-NAME "test_table" 

ADD FIELD "f1" OF "test_table" AS integer
  FORMAT "->>,>>9" 
  INITIAL "0" 
  POSITION 2
  MAX-WIDTH 312
  EXTENT 26
  ORDER 10

.
PSC
cpstream=ISO8859-1
.
0000000214

Error is displayed by the added error from the patch, FQLPreprocessor:5187.

I am also trying to find a test case that does not require this patch.

The constructor for FindQuery looks like this:

   public FindQuery(DataModelObject dmo,
                    String where,
                    Supplier<logical> whereExpr,
                    String sort,
                    DataModelObject inverse,
                    Object[] args,
                    LockType lockType)
   {
      super();
      super.initialize(dmo, where, whereExpr, sort, null, inverse, args, lockType);

      // if the NO-ERROR clause is specified, then a QOEE must not be thrown
      setLenientOffEnd(getBuffer().txHelper.errHlp.isSilent());
      setErrorIfNull(true);
   }

The super.initialize and other code needs to be executed in the find and silentFind versions, to be aware of the new 'silent' mode for FIND.

Constantin Asofiei wrote:

The super.initialize and other code needs to be executed in the find and silentFind versions, to be aware of the new 'silent' mode for FIND.

That's actually a better idea that what I was thinking. This should go on a separate branch (9032d) so I'll start working on it asap.

This code:

      // if the NO-ERROR clause is specified, then a QOEE must not be thrown
      setLenientOffEnd(getBuffer().txHelper.errHlp.isSilent());

Constantin Asofiei wrote:

This code:
[...]
is no longer included in initializeQuery.

It is not needed anymore, it is either false for non-silent operations or set directly in the silent mode method.

Thanks, I see it now. But I think the same can be achieved by overriding the first/last/etc methods in FindQuery, instead of using an empty default method initializeQuery in P2JQuery. It can get confusing that there is no impl for RAQ, and this is only for FIND anyway.

I think 9032d is good, we'll need to run a ETF test.

Constantin Asofiei wrote:

The changes are not complete. All unique/first/last/etc methods from RAQ need to be overridden. For example there is this:
[...]
which calls getCurrentArgs() and this needs to have initializeQuery already executed.

This is now fixed in 9032d/15414.

hasAny() calls also need to initialize the query.

Dănuț Filimon wrote:

hasAny() calls also need to initialize the query.

Also hasOne(). Any method emitted for FindQuery in conversion needs to have this.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

hasAny() calls also need to initialize the query.

Also hasOne(). Any method emitted for FindQuery in conversion needs to have this.

Yes, this also includes addExternalBuffers(). I was trying to look into the customer project I converted with 9032b, but I accidentally ran an ant deploy.all while I actually wanted to reimport the database.

Committed 9032d/15415.

ETF testing passed with 9032d/15415. Please do some basic testing of some other customer apps.

I smoke tested a customer application and found no issues.

Greg, 9032d can be queued for merge.

Constantin Asofiei wrote:

Greg, 9032d can be queued for merge.

Please merge to trunk now.

Branch 9032d was merged to trunk rev 15416 and archived.

Dănuț Filimon wrote:

Branch 9032d was merged to trunk rev 15416 and archived.

Does this address the error: code too large issue noted in #8860-66?

Danut is out-of-office until Monday.

Anyway, I don't think that #9032 itself will solve anything from "code too large". There is a 64kB byte limit per method. Danut's changes is basically generating (a bit) more byte-code for each FIND query.
AFAIK, FWD doesn't have anything like "automatically split code to satisfy Java constraints", so this will require changes in 4GL code: maybe split a procedure in different internal procedures? something like that.

Roger Borrello wrote:

Dănuț Filimon wrote:

Branch 9032d was merged to trunk rev 15416 and archived.

Does this address the error: code too large issue noted in #8860-66?

There is NOT a solution in FWD for this. See Code Too Large javac issue. The bottom line is that this code must be refactored on the 4GL side to split up the code into multiple procedures so that the individual methods are smaller than today.

This issue can appear at any time that we make changes to the conversion and some methods will just be "on the edge" of the problem (e.g. #8860-65). Something in the changes pushed that case "over the edge" and now some 4GL changes are needed. We don't see this issue in every project but it can happen a few times in any given project.

It has come to my attention that there is an issue with lenientOffEnd not being set for silent(() -> FindQuery().next()) calls. After 9032d was merged, the flag is no longer set in the constructor and this is done by the silent method so the runtime is no longer compatible with old conversions. Anything converted with trunk/15411 will require trunk/15416 to work, while any conversion that uses a revision older than 15411 will be compatible with the runtime up to 15415.

Rebased 9032a to latest trunk/15472, the branch is now at revision 15476.
Rebased 9032c to latest trunk/15472, the branch is now at revision 15473.