FWD » Core Development » Database

• a new FindQuery instance to be created
• state about the query to be re-built

• the Java method for a function, internal procedure or class method/constructor (static or not)
• the Java class for an external procedure as an instance field (emitting this as variable for the 'execute' Java method just means pushing this as an argument at the 'body' lambda call, and I would avoid this).

The FindQuery will re-use its internal state as long as the buffer(s) are the same - if these are not the same, it will get re-initialized. So this task requires both conversion changes and runtime changes, to be able to re-use the FindQuery instance.

I suggest assigning this to Danut after #9032 is finished (the part about 'silent' mode for FIND statements).

There is another point here. When a Java method gets called 10k's of times, then the local-var FIND will be created each time. This will not solve the main issue: the solution is to move all FIND instances at the external program (Java class fields, static or not, depending on where is used), and ensure compatibility with recursive calls. This would mean that the FindQuery instance needs to track 'nested mode' and if the call is recursive, then create another instance and not reuse the same FindQuery instance (at the class field); this needs to be managed by runtime.

• The number of total FindQuery instances created;
• How many FindQuery instance are used in a java method for a function/internal procedure/class method/constructor;
• How many FindQuery instances are used in external procedures ('execute' method);
• How many FindQuery instances were the buffers change/remain the same;
• How many FindQuery instances end up in recursive calls (this reminded me of #6709 where I added a nested find report, not sure if it's relevant as it only looks for nested FINDs in FOR EACH queries);

• A part of the analysis can be done on the converted code (how many FindQuery instances can be found in the application and where are those defined), my idea is to only focus on a customer scenario and monitor the runtime. Is this alright?
• In what scenario can the buffers of a FindQuery change (if we imagine that we already have #9078 implemented)?

Dănuț Filimon wrote:

• A part of the analysis can be done on the converted code (how many FindQuery instances can be found in the application and where are those defined), my idea is to only focus on a customer scenario and monitor the runtime. Is this alright?

You can have a single FIND statement in a block, and executed a million times - so yes, doing some profiling to see how many FIND queries are created is better.

• In what scenario can the buffers of a FindQuery change (if we imagine that we already have #9078 implemented)?

See the AdaptiveFind issue: a buffer can switch its bound buffer at any time. But otherwise, the proxy reference remains the same.

• Instances created: 6900539
• Instances created in a constructor call: 1022
• Instances created by execute method calls: 385039
• Instances created by other methods: 6514478 (excluding destructors - there were no calls for this type of methods)

The test was done on an extended performance scenario from a customer using 7156c/15502 (100 runs).

• Instances created: 6900552
• Instances created in a constructor call: 1022
• Instances created by execute method calls: 385039
• Instances created by other methods: 6514491

The top is based on the class name, method name and line on which the FindQuery is initialized.

What does first place mean? Is the same FIND Query 1022 times in a constructor, 111403 in an execute and 883905 in an internal procedure. How can a FindQuery from a specific class/method/line be in the constructor, execute and internal proc at the same time? I don't understand.

Alexandru Lungu wrote:

What does first place mean? Is the same FIND Query 1022 times in a constructor, 111403 in an execute and 883905 in an internal procedure. How can a FindQuery from a specific class/method/line be in the constructor, execute and internal proc at the same time? I don't understand.

I can't post the class names/methods because it is related to a customer. The tops are 'separated' from each other, when looking at the constructor calls there is a single constructor which initialized FindQuery instances in the same place 1022 times. FindQuery instances are either created from a constructor, execute method or function/internal procedure/class method and are distinct so there is no way the same FindQuery is initialized in both the constructor and execute method.

I think I get it ... there is no link between the columns. So on line 1, there are actual 3 different find queries.

TBH: 883905 is quite a large number :) In theory, extracting this as a class member should reduce the total count number. We can reduce to 1, if the query "blueprint" would be static - I think this is the goal anyway.

I am currently working on adding a "qryname" annotation to each find query (not just for the AdaptiveFind) but I am facing a problem where I am adding the annotation in Code Conversion Annotations step but it is no longer available in the Core Code Conversion step.

Dănuț Filimon wrote:

I am currently working on adding a "qryname" annotation to each find query (not just for the AdaptiveFind) but I am facing a problem where I am adding the annotation in Code Conversion Annotations step but it is no longer available in the Core Code Conversion step.

The annotation is "removed" in the same Code Conversion Annotations step. I quoted removed because there is no removeAnnotation() or cleanAnnotations() method being called at all.

• Before
  

     @LegacySignature(type = Type.MAIN, name = "start.p")
     public void execute()
     {
        externalProcedure(Start.this, new Block((Body) () -> 
        {
           RecordBuffer.openScope(tt1);
           tt1.create();
           tt1.setF1(new integer(1));
           tt1.release();
           new FindQuery(tt1, (String) null, null, "tt1.recid asc").first();
        }));
     }
  

• After
  

     @LegacySignature(type = Type.MAIN, name = "start.p")
     public void execute()
     {
        FindQuery query0 = new FindQuery();
  
        externalProcedure(Start.this, new Block((Body) () -> 
        {
           RecordBuffer.openScope(tt1);
           tt1.create();
           tt1.setF1(new integer(1));
           tt1.release();
           query0.initialize(tt1, ((String) null), null, "tt1.recid asc").first();
        }));
     }
  

I know that the query should be defined as an instance field because of the execute method (this is WIP).

Committed my changes to 9078a/15517.

Committed 9078a/15518. Added a default constructor for FindQuery so that the converted code compiles.

I am not sure if the FindQuery should be generated as a class member anyway. Its scope can remain there. Otherwise, for a persistent procedure, we will "carry" that find query till the end of life of the procedure. It doesn't make sense as the query would be used only inside the external scope anyway. I think it can remain there even for excute cases, because internal procedures don't need that.

Alexandru Lungu wrote:

I am not sure if the FindQuery should be generated as a class member anyway. Its scope can remain there. Otherwise, for a persistent procedure, we will "carry" that find query till the end of life of the procedure. It doesn't make sense as the query would be used only inside the external scope anyway. I think it can remain there even for excute cases, because internal procedures don't need that.

Thanks! I was planning on leaving this at the end because the call

<action>execLib("define_and_construct", classname, qryname)</action>

   @LegacySignature(type = Type.METHOD, name = "test19")
   @Test(order = 19)
   public void test19()
   {
      object<? extends com.goldencode.p2j.oo.lang.LegacyError> ex_top = TypeFactory.object(com.goldencode.p2j.oo.lang.LegacyError.class);

      internalProcedure(Test9032Fqnext1.class, this, "test19", new Block((Init) () -> 
      {
         catchError(com.goldencode.p2j.oo.lang.LegacyError.class, 
         ex -> 
         {
            ex_top.assign(ex);
            Assert.isTrue(new logical(false), new character("Should not get exception - test19"));
         });
      }, 
      (Body) () -> 
      {
         FindQuery query18 = new FindQuery();

         OnPhrase[] onPhrase0 = new OnPhrase[]
         {
            new OnPhrase(Condition.ERROR, Action.THROW, "blockLabel0")
         };

         doBlock(TransactionType.FULL, "blockLabel0", onPhrase0, new Block((Body) () -> 
         {
            {
               query18.initialize(tt46, ((String) null), null, "tt46.recid asc").next();
            }
         }));

         Assert.isFalse(tt46.available());
      }));
   }

Another known problem where the FindQuery is not properly converted:

   @LegacySignature(type = Type.METHOD, name = "test1")
   @Test(order = 1)
   public void test1()
   {
      object<? extends com.goldencode.p2j.oo.lang.LegacyError> ex_top = TypeFactory.object(com.goldencode.p2j.oo.lang.LegacyError.class);

      internalProcedure(Test9032Fqcurrent1.class, this, "test1", new Block((Init) () -> 
      {
         catchError(com.goldencode.p2j.oo.lang.LegacyError.class, 
         ex -> 
         {
            ex_top.assign(ex);
            Assert.isTrue(new logical(false), new character("Should not get exception - test1"));
         });
      }, 
      (Body) () -> 
      {
         new FindQuery().initialize(tt1, ((String) null), null, "tt1.recid asc").current();
         Assert.isFalse(tt1.available());
      }));
   }

I have trouble differentiating between the top block execute method and top block class method. Because of #9078-20 and how 9078a is implemented, it find the first available top level block but if there are multiple nested blocks it should search for the farthest block control point. This is a problem because the farthest block will always be the execute method of the class, converting to:

   @LegacySignature(type = Type.EXECUTE)
   public void __unit_Test9032Fqcurrent3_execute__()
   {
      RecordBuffer.openScope(tt11);
      onRoutineLevel(Condition.ERROR, Action.THROW);
      {
      }
      FindQuery query0 = new FindQuery();
      FindQuery query1 = new FindQuery();
      FindQuery query2 = new FindQuery();
      FindQuery query3 = new FindQuery();
      FindQuery query4 = new FindQuery();
      ...

I've managed to fix #9078-21, there was another rule whre the FindQuery constructor was created so I had to change it to generate a variable. Still working on #9078-20 and #9078-22 problem.

I've managed to find a fix for #9078-22, it involved searching for a top level block that has a top level ancestor and this will return a null for the cases where there are none (execute method) and will try searching for the current top level block that corresponds to the class.

I also found an additional scenarios that I overlooked, CAN-FIND:

   @LegacySignature(type = Type.METHOD, name = "TestFunctionality")
   @Test(order = 1)
   public void testFunctionality()
   {
      internalProcedure(TestCanFind.class, this, "TestFunctionality", new Block((Body) () -> 
      {
         Assert.isFalse(new FindQuery(ttitem, (String) null, null, "ttitem.recid asc", LockType.NONE).hasOne());     
      }, 
      (Fini) () -> 
      {
         ttitem.deleteAll();
      }));
   }

Committed 9078a/15519. Support FindQuery constructor conversion as a variable scoped to the top level block.

TODO: Improve rules

Next step: CAN-FIND conversion.

Committed 9078a/15520. Modified CAN-FIND conversion to match the FIND statement conversion.

I discovered a new conversion issue while testing hotel_gui so I am currently investigating it.

Dănuț Filimon wrote:

I discovered a new conversion issue while testing hotel_gui so I am currently investigating it.

The issue is related to choosing the right query name in some cases. For example using a DO PRESELECT where there is no FindQuery instance created (it is a PreselectQuery). I had my concerns regarding this, I'll have to make sure that the query names are generated and used correctly.

Dănuț Filimon wrote:

Dănuț Filimon wrote:

I discovered a new conversion issue while testing hotel_gui so I am currently investigating it.

The issue is related to choosing the right query name in some cases. For example using a DO PRESELECT where there is no FindQuery instance created (it is a PreselectQuery). I had my concerns regarding this, I'll have to make sure that the query names are generated and used correctly.

Fixed this issue in 9078a/15521. The solution was to assign the query name a bit later, enough for the block query annotation to be created. This annotation is checked before a new one is created for the FIND statement.

9078a/15521 commit reverts most of the effort done for 9078a. I will have to think of another solution.

Dănuț Filimon wrote:

9078a/15521 commit reverts most of the effort done for 9078a. I will have to think of another solution.

I reverted the changes from 9078a/15521 in 9078a/15522. Currently working on another solution.

• AddRoomsDialog.execute():
  

     @LegacySignature(type = Type.MAIN, name = "add-rooms-dialog.w", parameters = 
     {
        @LegacyParameter(name = "needRefresh", type = "LOGICAL", mode = "OUTPUT")
     })
     public void execute(final logical _needRefresh)
     {
        externalProcedure(AddRoomsDialog.this, new Block((Init) () -> 
        {
           ...
        }, 
        (Body) () -> 
        {
           registerTrigger(new EventList("CHOOSE", gdialogFrame.widgetBtnOk()), AddRoomsDialog.this, () -> new Trigger()
           {
              ...
              FindQuery query0 = new FindQuery();
  
              public void body()
              {
              }
           }
        }
     }
  

• GuestsFrame.execute():
  

           FindQuery query1 = new FindQuery();
  
           registerTrigger(new EventList("CHOOSE", fmainFrame.widgetInvoiceButton()), GuestsFrame.this, new Trigger((Body) () -> 
           {
              ...
  
              query1.initialize(xrptStay, "xrptStay.recid = ?", null, "xrptStay.stayId asc", new Object[]
              {
                 (P2JQuery.Parameter) () -> stay.recordID()
              }).first();
              ...
           }));
  

This one also:

   @LegacySignature(type = Type.MAIN, name = "start.p")
   public void execute()
   {
      externalProcedure(Start.this, new Block((Body) () -> 
      {
         RecordBuffer.openScope(tt1);

         PreselectQuery query2 = new PreselectQuery();

         doBlock("blockLabel0", new Block((Init) () -> 
         {
            query2.initialize(tt1, ((String) null), null, "tt1.recid asc");
         }, 
         (Body) () -> 
         {
            query2.first();
         }));

         AdaptiveQuery query3 = new AdaptiveQuery();
         FindQuery query1 = new FindQuery();

         forEach(query3, "loopLabel0", new Block((Init) () -> 
         {
            query3.initialize(tt1, ((String) null), null, "tt1.recid asc");
         }, 
         (Body) () -> 
         {
            query1.initialize(tt1, ((String) null), null, "tt1.recid asc").first();
         }));
      }));
   }

define temp-table tt1 field f1 as integer.

do preselect each tt1.
   find first tt1.
end.

for each tt1.
   find first tt1.
end.

Committed 9078a/15523. Added the fix mentioned in #9078-32.

Fixed #9078-33 in 9078a/15524.

• FIND statements are annotated using "fqryname" annotation instead of "qryname". Because those statements are processed much earlier, there are unrelated queries that receive a different query name so comparing conversion is much harder.
• For AdaptiveFind, annotating "qryname" takes priority over "fqryname" so it this kind of situations it will be removed.

1. Can you clarify the cases where we should keep the old conversion? You mentioned that #9032-33 should not emit the FindQuery outside of the FOR EACH block and that the GuestsFrame.execute() from #9032-32 should not emit the FindQuery outside of the registerTrigger while the AddRoomsDialog.execute() is fine. I want to make sure I have a clear understanding of all scenarios and none show up later (at this moment all FindQuery constructors are emitted at the beginning of each method, while the #9032-32 remained as it is).
2. What stops us from doing a cleanup of the FindQuery and reusing it in cases like #9078-33?

Rebased 9078a with trunk/15526, the branch is now at revision 15535.

Dănuț Filimon wrote:

Constantin, at this point the conversion changes for #9032-2 are implemented, but during yesterday's standup you mentioned there are some cases related to INNER_BLOCK where we should keep the old conversion. My questions are:

1. Can you clarify the cases where we should keep the old conversion? You mentioned that #9032-33 should not emit the FindQuery outside of the FOR EACH block and that the GuestsFrame.execute() from #9032-32 should not emit the FindQuery outside of the registerTrigger while the AddRoomsDialog.execute() is fine. I want to make sure I have a clear understanding of all scenarios and none show up later (at this moment all FindQuery constructors are emitted at the beginning of each method, while the #9032-32 remained as it is).

No, the point is: if you have a FIND for which its nearest block parent is an INNER-BLOCK (which is also a loop), then extract the FindQuery instance at the nearest top-level block (either as a var def at the Java method or inside the top-level block, doesn't matter as long is outside of all parent loop blocks).

1. What stops us from doing a cleanup of the FindQuery and reusing it in cases like #9078-33?

When a FIND is used directly in the top-level block (so there is no 'looping block' as parent), that FIND instance can not ever be re-used - is a 'one time and done'. If you extract it as a Java method var, it doesn't help or hurt. The point was to keep the code 'clean' to be easier to check for conversion changes. Also, just a reminder, we can't promote a FindQuery scoped to an internal procedure/function as a class field because of recursivity.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Constantin, at this point the conversion changes for #9032-2 are implemented, but during yesterday's standup you mentioned there are some cases related to INNER_BLOCK where we should keep the old conversion. My questions are:

1. Can you clarify the cases where we should keep the old conversion? You mentioned that #9032-33 should not emit the FindQuery outside of the FOR EACH block and that the GuestsFrame.execute() from #9032-32 should not emit the FindQuery outside of the registerTrigger while the AddRoomsDialog.execute() is fine. I want to make sure I have a clear understanding of all scenarios and none show up later (at this moment all FindQuery constructors are emitted at the beginning of each method, while the #9032-32 remained as it is).

No, the point is: if you have a FIND for which its nearest block parent is an INNER-BLOCK (which is also a loop), then extract the FindQuery instance at the nearest top-level block (either as a var def at the Java method or inside the top-level block, doesn't matter as long is outside of all parent loop blocks).

1. What stops us from doing a cleanup of the FindQuery and reusing it in cases like #9078-33?

When a FIND is used directly in the top-level block (so there is no 'looping block' as parent), that FIND instance can not ever be re-used - is a 'one time and done'. If you extract it as a Java method var, it doesn't help or hurt. The point was to keep the code 'clean' to be easier to check for conversion changes. Also, just a reminder, we can't promote a FindQuery scoped to an internal procedure/function as a class field because of recursivity.

Thanks for the clarification! My next step is to create more test cases and investigate the conversion in such cases.

Danut, the FindQuery var def should be similar to the OPEN QUERY var def - see convert/database_access.rules around line 986. Maybe this helps.

Constantin Asofiei wrote:

Danut, the FindQuery var def should be similar to the OPEN QUERY var def - see convert/database_access.rules around line 986. Maybe this helps.

It does help, I think it is even simpler than what I've done until now. I'll have to try a few things right away, thanks!

I've managed to get a stable conversion with the new changes, but I found this case that needs to be investigated:

               ToClause toClause0 = new ToClause(i, firstVal, lastVal);

               repeatTo(TransactionType.FULL, "loopLabel1", toClause0, new Block((Body) () -> 
               {
                  new FindQuery(room, "room.roomNum = ?", null, "room.roomNum asc", new Object[]
                  {
                     i
                  }).silentFirst();

                  ...
               }));

Committed 9078a/15537. Fixed wrong order of emitted FindQuery variable.

Committed 9078a/15538.

• Extended field initialization in FindQuery to support the new conversion changes. Fields are initialized through a initializeFields() method (can't name it initialize() because of the ambiguity with a RandomAccessQuery method)

Next step: Investigate the condition to determine if the used buffers are the same (I suspect the same should be done for the DMO used by the query in a join via a foreign relation).

• hasAncestorLoopingInnerBlock - this needs to look through all 'inner_block', not just the immediate one, until the nearest top-level block is reached
• in database_access.rules, this code:
  

                                <while>
                                   toplvlref != null                                   and
                                   !(toplvlref.isAnnotation("toplevel")                and
                                     #(boolean) toplvlref.getAnnotation("toplevel"))
                                   <!-- walk up the tree -->
                                   <action>toplvlref = toplvlref.parent</action>
                                </while>
  

  
  I think can be replaced with getAncestor(null, "toplevel", true, prog.block).
  • is it ever possible for <rule>toplvlref != null and toplvlref.isAnnotation("instvarid") to be false? I.e. an external program which just a REPEAT: FIND... ?
  • you have this code:
    

                               <action>
                                  qryid = tw.graftAt("peer_method_call", copy,
                                                     location, -1,
                                                     "javaname", qryname,
                                                     "methodtxt", "initialize").id
                               </action>-->
    

    • there is a wrong? --> char?
    • what would be location here?
  • the code between lines 2003-2017 and 2032-2045 is duplicated. Why not calculate the 'top level ref' once and after that do the work?
  • this condition:
    

                         <rule on="false">qryname == null
                            <!-- no qryname means that there is no query to assign to (e.g.
                                 FindQuery) -->
                            <action>
                               qryid = createPeerAst(java.constructor, classname, location)
                            </action>
    

    
    was moved outside of the rule from line 1950 to inside the rule on line 1994 (<rule on="false">classname.equals("FindQuery")), and previously condition was <rule on="false">qryname == null. I'm not sure this is the same as it worked previously.

From my understanding, there are no longer queries without a name. Each FindQuery will have a name, even if it is not emitted as a variable so the qryname null was not needed anymore as there was no case I could think of where a query does not have a name (maybe a CompoundQuery component?).

Just a quick hint: CAN-FIND statements were previously generated without a name, because they were inlined. For instance a = CAN-FIND(tt) would generate a FindQuery, but won't assign it to a variable because the condition doesn't allow the definition of queries ad-hoc. If you took care of CAN-FIND queries, I think I am ok with this.

Alexandru Lungu wrote:

From my understanding, there are no longer queries without a name. Each FindQuery will have a name, even if it is not emitted as a variable so the qryname null was not needed anymore as there was no case I could think of where a query does not have a name (maybe a CompoundQuery component?).

Just a quick hint: CAN-FIND statements were previously generated without a name, because they were inlined. For instance a = CAN-FIND(tt) would generate a FindQuery, but won't assign it to a variable because the condition doesn't allow the definition of queries ad-hoc. If you took care of CAN-FIND queries, I think I am ok with this.

I also assigned a query name to CAN-FIND queries.

Committed 9078a/15539. Addressed #9078-47.

I think something like

<action on="false">toplvlref = copy.getAncestor(-1, prog.block, "toplevel", true)</action>

Fixed an action and removed the check in 9078a/15540.

Constantin Asofiei wrote:

See the AdaptiveFind issue: a buffer can switch its bound buffer at any time. But otherwise, the proxy reference remains the same.

Constantin, does this mean that I have to check the initialized buffer (RandomAccessQuery.buffer) so that it matches the buffer of the dmo? Also, how should I handle the inverse used to build the join? I suspect the DynamicJoin/DynamicLegacyKeyJoin will need to be rebuilt (in the case where the inverse buffer does not match but the dmo buffer does - if possible).

The only way to initialize the FIND is via FindQuery.initialize, right? All DMOs (the proxy) are there, save them as 'orig' and compare the bound buffer(s) the next time this gets initialized - if they differ, then re-initialize the instance. There may be some reset code to execute, to get the FindQuery in the 'initial' state, when initialization is not done.

Constantin Asofiei wrote:

The only way to initialize the FIND is via FindQuery.initialize, right?

Not quite, the fields are can be initialized through the constructor but also an initialize method (but I am making changes so that it reaches the same method - initializeFields). The method that actually does the initialization is initializeQuery.

All DMOs (the proxy) are there, save them as 'orig' and compare the bound buffer(s) the next time this gets initialized - if they differ, then re-initialize the instance. There may be some reset code to execute, to get the FindQuery in the 'initial' state, when initialization is not done.

I will be looking into it, thanks!

Dănuț Filimon wrote:

Constantin Asofiei wrote:

The only way to initialize the FIND is via FindQuery.initialize, right?

Not quite, the fields are can be initialized through the constructor but also an initialize method (but I am making changes so that it reaches the same method - initializeFields). The method that actually does the initialization is initializeQuery.

Well, for the 'promoted' FindQuery instances, the constructor will not receive anything, so you can mark these with some flag to distinguish the optimized and non-optimized instances.

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Constantin Asofiei wrote:

The only way to initialize the FIND is via FindQuery.initialize, right?

Not quite, the fields are can be initialized through the constructor but also an initialize method (but I am making changes so that it reaches the same method - initializeFields). The method that actually does the initialization is initializeQuery.

Well, for the 'promoted' FindQuery instances, the constructor will not receive anything, so you can mark these with some flag to distinguish the optimized and non-optimized instances.

I did think of a nested flag (because of #9078-3), sounds like a plan!

I managed to successfully convert a large customer application and after looking a bit of the sources I did not notice anything wrong (there are also a lot of changes so this makes it a bit harder).

One thing that I want to mention is that there were no error: code too large problems and an ongoing "issue". You can have a converted file and it only has a single fquery22 defined (it would normally start from 0), but the other names are claimed by existent FindQuery instances that did not benefit from this promotion.

Constantin, do you have any test case where the bound buffers can differ? I tried getting a snippet from a customer application that I am testing and changing it, but now the application is no longer working (not sure why) so I am planning to run a full conversion again.

Danut, the scenario would be like this, but OE doesn't allow unbind/re-bind of an already bound reference-only table see also https://community.progress.com/s/article/000057279:

def temp-table tt1 field f1 as int.
def temp-table tt2 field f1 as int.
def temp-table tt3 reference-only field f1 as int.

procedure proc0.
  def input param table for tt3 bind.
end.

procedure proc1.
  def input param table for tt3 bind.

end.

def var l as log.
l = true.
repeat:

   if l then run proc0(input table tt1 bind).
    else run proc1(input table tt2 bind).
   message temp-table tt3:name.
   l = not l.
   find first tt3 no-error.
   pause.
end.

I think I might have been chasing a totally different bug in a customer application (a bit weird), the following query:

         PresortQuery query5 = new PresortQuery();
         FieldReference byExpr1 = new FieldReference(ttable1, "field1");
         FieldReference byExpr2 = new FieldReference(ttable1, "field2");

         forEach(query5, "forLoop4", new Block((Init) () -> 
         {
            RecordBuffer.openScope(bufttableBuf2);
            query5.initialize(ttable1, "ttable1.field4 = true and ttable1.field5 = 1", null, "ttable1.field1 asc, ttable1.field3 asc");
            query5.enableBreakGroups();
            query5.setNonScrolling();
            query5.addSortCriterion(byExpr1);
            query5.addSortCriterion(byExpr2);
         }, 
         (Body) () -> 
         {
            ...
         }

There is an extra loop that starts with the following record in ttable1 buffer:

local/_temp/primary Ttable1_1 (ttable1@10:7#1) [tt195]
---------------------------------------------
tt195:3076
RecordState{NOUNDO CACHED }
references: 1
dirty: {}
unvalidated: {}
---------------------------------------------
   recid                :  3076
   __error-flag__       :  ?
   __origin-rowid__     :  ?
   __datasource-rowid__ :  ?
   __error-string__     :  ?
   __after-rowid__      :  ?
   __row-state__        :  ?
   ...
   field5               :  ?
   ...
   field4               :  true
   ...

I am using the trunk/15523 sources, while 9078a is based on trunk/15526. Right now I plan to use trunk/15526 and check if this error appears, then remove these 3 revisions (just for testing and this should be ok as those revisions only contain runtime changes), rebase the branch and run a new conversion if necessary.

#9078-63 can be ignored, after looking into the structure of the table there is an additional field similar to field5 and that is ffield5.

I've converted ETF with 9078a but all tests fail due to Cannot create temp table field for "BATCHACTION". I've also tried rebasing 9078a but there were recent changes done to the FindQuery to support multi-tenancy and it turned into game of cherry picking. My changes were not done with multi-tenancy in mind, so I'll probably have to create a new branch, then port and fix everything there.

I created 9078b from trunk/15621, after making a patch of 9078a on top of that branch I found conversion issues using my old tests. Currently working on fixing the tests, then I'll commit my changes.

The following FindQuery extracted from ETF (slightly modified table/field names):

               fquery26.initialize(tt1, "tt1.f1 = ? and upper(tt1.f2) = ?", null, "tt1.f1 asc, tt1.f2 asc", new Object[]
               {
                  if1,
                  (P2JQuery.Parameter) () -> toUpperCase(entry(itablef1, subscript(cget, if1)))
               }).silentFirst();

1. First iteration, FindQuery is initialized -> initialized flag set to true
2. Find the record -> continue the execution flow
3. Second iteration, query is already initialized (flag) so the arguments are not updated.

• Reinitialize the query when the arguments do not match.

Dănuț Filimon wrote:

Solution:

• Reinitialize the query when the arguments do not match.

I found that even if the args are different and the query reinitialized, the RandomAccessQuery.currentArgs will still retain the old values! I only found this after looking into some tests that took considerably longer to execute (this can also result into infinite loops).

Example of the problem:

   @LegacySignature(type = Type.PROCEDURE, name = "test1")
   @com.goldencode.p2j.testengine.api.Test(order = 1)
   public void test1()
   {
      integer iicomtypid = TypeFactory.integer();
      FindQuery fquery0 = new FindQuery();

      internalProcedure(Test9078Arguments.this, "test1", new Block((Body) () -> 
      {
         iicomtypid.assign(10);

         repeat("loopLabel2", new Block((Body) () -> 
         {
            fquery0.initialize(tt90781, "tt90781.tableno = ?", null, "tt90781.recid asc", new Object[]
            {
               iicomtypid // contains the REFERENCE to the iicomtypid value, when it is updated the old arguments will be the new arguments, the query will not be reinitialized and the value stored in @currentArgs@ will still be 10
            }).silentFirst();
            Assert.isTrue(tt90781.available());

            if (_isEqual(tt90781.getTableno(), tt90781.getTableno1()))
            {
               leave("loopLabel2");
            }

            iicomtypid.assign(tt90781.getTableno1()); // updates the value after the first iteration
         }));

         Assert.legacyEquals(new integer(1), iicomtypid);
      }));
   }

Committed 9078b/15623. Fix an infinite loop by making arguments part of the condition for reinitialization, some arguments are passed by reference and a copy of the argument must be done in such cases. after digging into it.

It seems that the performance drop I was seeing was actually due to an infinite loop, this issue is now fixed. One more test, then I can consider step 1 and 3 from #9078-67 complete.

Managed to setup the dataset project to use multi-tenancy and convert a few examples. The conversion is regressing with 9078b and I am currently investigating.

While working on a few tests, I found another conversion regression when using multi-tenancy where a find query that can be emitted at the top level block will reach this rule in database_access.rules:

                        <!-- When processing a [mt-cross-database] tainted [RECORD_PHRASE] node, just emit
                             [setMultiTenantAlternative], after [addExternalBuffers]. All the above methods
                             were emitted by the sibling [RECORD_PHRASE] which doesn't have the 
                             annotation -->
                        <rule on="false">true
                           <action>printfln("parent: %s", parent.dumpTree(true))</action>
                           <action>extbufid = #(long) parent.getAnnotation("extbufid")</action>
                           <action>
                              qryid = tw.graftAt("peer_method_call", copy, getAst(extbufid).parent.id, -1,
                                                 "javaname",  qryname,
                                                 "methodtxt", "setMultiTenantAlternative").id
                           </action>
                        </rule>

     [java] FIND [KW_FIND]:21474836742 @24:9
     [java]       (fqryname=fquery0, support_level-byrule=gaps/gap_analysis_marking.xml:435, preselect_fetch=false, adaptive=false, support_level=16400, preselect_fetch-byrule=annotations/preselect_prep.rules:382, fqryname-byrule=annotations/database_general.rules:439, adaptive-byrule=annotations/adaptive_query.rules:321)

Fixed the problem from #9078-75 in 9078b/15625. The rules rely on the "extbufid" annotation to emit the setMultiTenantAlternative method for all types of queries, but FindQueries can use the location directly.

Rebased 9078b to latest trunk/15969, the branch is now at revision 15973.

Artur ran a conversion for a customer application and found a conversion issue. I plan to work with Artur to identify the root cause and create a test case, the problematic file did contain more than 450 find queries so I'll have to make sure that the right methods are used.

I discussed with Artur and took time to take a look at all find queries from the problematic class. I took each one and there is a matching initialize() method being used. The compile error given is confusing, as it doesn't specify anything related to the find query:

compile:
    [javac] Compiling 1725 source files to /home/as/projects/<placeholder>/build/classes
    [javac] /home/as/projects/<placeholder>/<placeholder>.java:1: error: too many parameters
    [javac] /*v****************************************************************************
    [javac] ^
    [javac] Note: Some input files use or override a deprecated API.
    [javac] Note: Recompile with -Xlint:deprecation for details.
    [javac] Note: Some input files use unchecked or unsafe operations.
    [javac] Note: Recompile with -Xlint:unchecked for details.
    [javac] 1 error 

The next step would be to setup the customer application myself and reproduce the issue.

I did not take into account that there might be a limitation to the number of local variables that can be defined in a method. In the execute() method, there are more than 450 FindQuery variables, but I am not yet sure if this is an actual limit for local variables.

Dănuț Filimon wrote:

I did not take into account that there might be a limitation to the number of local variables that can be defined in a method. In the execute() method, there are more than 450 FindQuery variables, but I am not yet sure if this is an actual limit for local variables.

• refactor the customer code
• find another conversion strategy

Maybe you can create an array and instead of emitting FindQuery findQuery123 = new FindQuery(..);, emit findQuery[123] = new FindQuery(..). Please think of other approaches and lets discuss here.

Alexandru Lungu wrote:

Dănuț Filimon wrote:

I did not take into account that there might be a limitation to the number of local variables that can be defined in a method. In the execute() method, there are more than 450 FindQuery variables, but I am not yet sure if this is an actual limit for local variables.

Java has several limitations of that kind (number of methods a class can have, number of lines a method can have, etc.) maybe more. We also face this when converting code which ends up breaking these hard limits (which are not configurable). It is either:

• refactor the customer code
• find another conversion strategy

Maybe you can create an array and instead of emitting FindQuery findQuery123 = new FindQuery(..);, emit findQuery[123] = new FindQuery(..). Please think of other approaches and lets discuss here.

I like this idea. I need to note that all FindQueries have a name that is replaced when handling an AdaptiveFind query, at the same time all FIND and CAN-FIND statements need a query name which is not used all the time (FindQuery instances are only emitted for inner blocks that are also loops). The conclusion findQuery[123] will not match the fquery123. There's also the option of using a Map where the key will be the query name and the value will be the instance, I believe this will also make the code easy to read.

I'll check both ideas and see which ones works better.

Danut, just keep in mind that a Map has a higher overhead than a plain array. That is why I am suggesting creating an array instead.

I found the named_array template in java_templates.tpl and managed to generate something like:

      FindQuery fquery0 = new FindQuery();
      FindQuery[] fqueryArray = new FindQuery[]
      {
      }
;

      <rule>fqueryCounter &gt; 0 and
            toplvlref != null
         <action>
            tw.graft("named_array",
                              toplvlref,
                              olocation,
                              "classname",
                              "FindQuery",
                              "javaname",
                              "fqueryArray",
                              "extent",
                              5)
         </action>
         <action>toplvlref = null</action>
         <action>olocation = null</action>
      </rule>

I've adjusted the template in the following way:

   <!-- a named array initializer -->
   <ast-root name="named_extent_array" terse="true" ast_class="com.goldencode.p2j.uast.JavaAst" >
      <ast id="0" type="ASSIGN" text="">
         <ast id="0" type="REFERENCE_DEF" text="${classname}">
            <annotation datatype="java.lang.String" key="name" value="${javaname}" />
            <annotation datatype="java.lang.Long" key="extent" value="${extent}" />
         </ast>
         <ast id="0" type="EXPRESSION" text="">
            <ast id="0" type="REFERENCE_DEF" text="new ${classname}">
               <annotation datatype="java.lang.Long" key="extent" value="${extent}" />
            </ast>
         </ast>
      </ast>
   </ast-root>

The following template worked:

   <!-- a named array initializer -->
   <ast-root name="named_extent_array" terse="true" ast_class="com.goldencode.p2j.uast.JavaAst" >
      <ast id="0" type="ASSIGN" text="">
         <ast id="0" type="REFERENCE_DEF" text="${classname}">
            <annotation datatype="java.lang.String" key="name" value="${javaname}" />
            <annotation datatype="java.lang.Long" key="extent" value="${extent}" />
         </ast>
         <ast id="0" type="EXPRESSION" text="">
            <ast id="0" type="REFERENCE" text="new ${classname}">
               <annotation datatype="java.lang.Long" key="extent" value="${extent}" />
            </ast>
            <ast id="0" type="LBRACKET" text="[">
               <ast id="0" type="NUM_LITERAL" text="${extent}" />
            </ast>
         </ast>
      </ast>
   </ast-root>

The following test:

DEFINE BUTTON Btn_OK AUTO-GO
    LABEL "OK" .

DEFINE BUTTON Btn_Cancel AUTO-GO
    LABEL "CANCEL" .

DEFINE TEMP-TABLE tt1 FIELD f1 AS INTEGER.

DEFINE FRAME gdialog
   Btn_OK at row 8.67 col 2
   Btn_Cancel at row 8.67 col 21
   with view-as dialog-box keep-tab-order
      side-labels no-underline three-d scrollable
      DEFAULT-BUTTON Btn_OK cancel-button Btn_Cancel widget-id 100.

ON CHOOSE OF Btn_OK IN FRAME gdialog DO:
    DEF VAR firstVal AS INT INITIAL 1.
    DEF VAR lastVal AS INT INITIAL 3.
    DEF VAR i AS INT INITIAL 1.
    DEF VAR iCount AS INT.

    iCount = 0.
    REPEAT i = firstVal TO lastVal:
        FIND FIRST tt1 WHERE tt1.f1 = i NO-ERROR.
        IF AVAIL(tt1) THEN DO:
            MESSAGE "found".
        END.
    END.
END.

view frame gdialog.

   @LegacySignature(type = Type.MAIN, name = "test4.p")
   public void execute()
   {
      externalProcedure(Test4.this, new Block((Body) () -> 
      {
         gdialogFrame.openScope();
         RecordBuffer.openScope(tt1);
         registerTrigger(new EventList("CHOOSE", gdialogFrame.widgetBtnOk()), Test4.this, () -> new Trigger()
         {
            integer firstVal = UndoableFactory.integer((long) 1);

            integer lastVal = UndoableFactory.integer((long) 3);

            integer i = UndoableFactory.integer((long) 1);

            integer iCount = UndoableFactory.integer();

            FindQuery[] fqueryArray = new FindQuery[1];

            fqueryArray[0] = new FindQuery();

            public void body()
            {
               iCount.assign(0);

               ToClause toClause0 = new ToClause(i, firstVal, lastVal);

               repeatTo("loopLabel0", toClause0, new Block((Body) () -> 
               {
                  fqueryArray[0].initialize(tt1, "tt1.f1 = ?", null, "tt1.recid asc", new Object[]
                  {
                     i
                  }).silentFirst();

                  if (tt1._available())
                  {
                     message("found");
                  }
               }));
            }
         });

         gdialogFrame.view();
      }));
   }

The solution would be to move fqueryArray[0] = new FindQuery(); in the body(), emit the FindQuery normally without considering the array or simply not emitting it at all.

will the template above do to generate FindQuery[] fqueryArray = new FindQuery⁵; or is there a way to make it simpler?

This approach is good as proposed. Although, I guess you may not need it now.

I was thinking about fqueryArray[0] = new FindQuery(); initialization. To have a more compact code, we could emit it like this:

      FindQuery[] fqueryArray = new FindQuery[] {
          new FindQuery(),
          new FindQuery(),
      }

      FindQuery[] fqueryArray = new FindQuery[];
      // ...
      for (int k = 0; k < fqueryArray.length; k++) {
          fqueryArray = new FindQuery();
      }

However, the chances are that in a big procedure, the queries are executed on different if or switch branches or never executed because of quick or premature return statements (or exception/error conditions). In these cases the initializations would become ballast. In this case the better solution is to create them lazily, uindividually, as close to the usage point.

Ovidiu Maxiniuc wrote:

I was thinking about fqueryArray[0] = new FindQuery(); initialization. To have a more compact code, we could emit it like this:[...]if the number of queries is small (let's say 5?). Then for a larger number we may want to initialize the array in a loop, like this:[...]

However, the chances are that in a big procedure, the queries are executed on different if or switch branches or never executed because of quick or premature return statements (or exception/error conditions). In these cases the initializations would become ballast. In this case the better solution is to create them lazily, uindividually, as close to the usage point.

Creating the FindQuery as close to the usage point is a great idea. I think it would be the best to generate it at the furthest block that is not a looping inner block. So if I have two looping inner blocks, it will look for a parent block that can allow the FindQuery to be emitted there.

I've made some progress, but I will have to refactor all of the changes for the FindQuery. If I am using an array to define the instances, even if just one, it will make things a little bit simpler and I can go ahead and remove FindQuery naming (fquery0..n). I have the templates in place and it partially works, but the only issue is the conflict of the array definition with the initial variables that are being emitted. Somehow I can't generate the array without emitting the variables, even if the array being created is not dependent on those variables. I will have to look into this more and check where's the problem.

Constantin mentioned that I could count the number of FindQueries for each top level block and emit the array only when necessary, I looked into writing a set of rules for this but I am stuck at the ascent rule for adding a fquery-counter annotation. I am working on getting this running.

Current rule-set:

<rule-set input="tree">
   <worker class="com.goldencode.p2j.uast.ProgressPatternWorker" namespace="prog" />

   <variable name="queue"       type="java.util.Deque" />

   <init-rules>
      <rule>queue = create("java.util.ArrayDeque")</rule>
   </init-rules>

   <walk-rules>
      <rule>(evalLib("isTopLevelInternalNode", this)        or
             (this.type == prog.block                       and
              #(boolean) this.getAnnotation("toplevel")))
            <action>queue.push(#(long) 0)</action>
      </rule>

      <rule>
         not queue.isEmpty() and
         (type == prog.statement                                               and
         (downPath(this, prog.kw_find, prog.record_phrase)                    or
          downPath(this, prog.kw_find, prog.kw_last)                          or
          downPath(this, prog.kw_find, prog.kw_prev)                          or
          downPath(this, prog.kw_find, prog.kw_next)                          or
          downPath(this, prog.kw_find, prog.kw_first))) or
          (type == prog.func_logical and getNoteLong("oldtype") == prog.kw_can_find)
         <action>queue.push(#(long) queue.pop() + 1)</action>
      </rule>
   </walk-rules>

   <ascent-rules>
      <!-- the following rule is wrong -->
      <rule> not queue.isEmpty()
            (evalLib("isTopLevelInternalNode", this)        or
             (this.type == prog.block                       and
              #(boolean) this.getAnnotation("toplevel")))
            <action>copy.putAnnotation("fquery-counter", #(long) queue.pop())</action>
      </rule>
   </ascent-rules>

</rule-set>

I managed to get a working version for #9078-95 and started working on a version where it uses an array in case too many FindQuery instances are defined. But I am facing an issue again and again, for the case below:

   @LegacySignature(type = Type.CONSTRUCTOR, name = "ConstructorTest2")
   public void __constructorTest2_constructor__()
   {
      FindQuery fquery0 = new FindQuery();
      FindQuery fquery1 = new FindQuery();
      FindQuery fquery2 = new FindQuery();
      FindQuery fquery3 = new FindQuery();
      FindQuery fquery4 = new FindQuery();
      FindQuery fquery5 = new FindQuery();
      FindQuery fquery6 = new FindQuery();
      FindQuery[] fqueryArray = new FindQuery[7];

      internalProcedure(ConstructorTest2.class, this, "ConstructorTest2", new Block((Body) () -> 
      {
         __lang_BaseObject_constructor__();

         AdaptiveQuery query0 = new AdaptiveQuery();
         forEach(query0, "loopLabel0", new Block((Init) () -> 
         {
            query0.initialize(tt1, ((String) null), null, "tt1.recid asc");
         }, 
         (Body) () -> 
         {
            {
               fquery0.initialize(tt2, ((String) null), null, "tt2.recid asc").first();
               fquery1.initialize(tt2, ((String) null), null, "tt2.recid asc").last();
               fquery2.initialize(tt2, ((String) null), null, "tt2.recid asc").current();
               fquery3.initialize(tt2, ((String) null), null, "tt2.recid asc").first();
               fquery4.initialize(tt2, ((String) null), null, "tt2.recid asc").last();
               fquery5.initialize(tt2, ((String) null), null, "tt2.recid asc").current();
               fquery6.initialize(tt2, ((String) null), null, "tt2.recid asc").first();
            }
         }));
      }));
   }

Dănuț Filimon wrote:

I managed to get a working version for #9078-95 and started working on a version where it uses an array in case too many FindQuery instances are defined. But I am facing an issue again and again, for the case below:
[...] It does not matter if the array is generated anywhere else (walk rules, post rules, ascent rules)... The moment I get rid of the fquery0...n and those are no longer constructed, the array can no longer be built. I thought that this is because at the time I am creating the array in ascent, the "instvarid" is not present, so I used a queue to store the ids and create the array at the end but it did not help. I have no lead on how to get the array to generate correctly when the separate constructors should not be used, I tried experimenting with ids/control points and I am not sure how to continue.

The only way I found to avoid this is to check each time if an array should be generated when I also generate an array entry.

I am currently investigating how to place the array entry as a child before the block that caused it to generate (without breaking the order of other constructs).

Rebased 9078b to latest trunk/16003, the branch is now at revision 16008.

I looked at 9078b / r16008.

The code looks pretty much solid. But it's difficult to spot all possible issues only by comparing the files in meld. Serious regression tests is required. The code is expected to be changed (in many places), the individual locations of these queries must be analysed manually.

• find_processing.rules:
  • lines 94-97 should be indented one more column since they are OR -ed with the downPath() at line 93.
  • also related to above lines: there is no handling for current and unique find s. Shouldn't we handle them the same?
• database_general.rules
  • lines 272-279: this is a side-effect of a function so the behaviour might not be expected. It should be put right near the function calls, in query_associations.rules.
• database_access.rules
  • line 2191: embedded comments within tag text is not pretty well supported in TRPL. Therefore I think the section after the comment (line 2192) is probably not processed by the TRPL interpreter.
• FindQuery.java
  • lines 230, 835, field nested is not in use. Can we drop it?

Due to nature of the TRPL language, there might be other issues I fail to spot. A second pair of eyes from Constantin would be welcomed.

Ovidiu Maxiniuc wrote:

I looked at 9078b / r16008.

The code looks pretty much solid. But it's difficult to spot all possible issues only by comparing the files in meld. Serious regression tests is required. The code is expected to be changed (in many places), the individual locations of these queries must be analysed manually.

Here are the issues I could spot:

• find_processing.rules:
  • lines 94-97 should be indented one more column since they are OR -ed with the downPath() at line 93.
  • also related to above lines: there is no handling for current and unique find s. Shouldn't we handle them the same?
• database_general.rules
  • lines 272-279: this is a side-effect of a function so the behaviour might not be expected. It should be put right near the function calls, in query_associations.rules.
• database_access.rules
  • line 2191: embedded comments within tag text is not pretty well supported in TRPL. Therefore I think the section after the comment (line 2192) is probably not processed by the TRPL interpreter.
• FindQuery.java
  • lines 230, 835, field nested is not in use. Can we drop it?

Due to nature of the TRPL language, there might be other issues I fail to spot. A second pair of eyes from Constantin would be welcomed.

I addressed the review in 9078b/16009, current and unique find s are already handled by the following rule:

+      <!-- Count all FIND and CAN-FIND statements -->
+      <rule>not blockQueue.isEmpty()                                                     and
+            (type == prog.statement                                                      and
+             (downPath(this, prog.kw_find, prog.record_phrase)                           or  <-- here
+              downPath(this, prog.kw_find, prog.kw_last)                                 or
+              downPath(this, prog.kw_find, prog.kw_prev)                                 or
+              downPath(this, prog.kw_find, prog.kw_next)                                 or
+              downPath(this, prog.kw_find, prog.kw_first)))                              or
+            (type == prog.func_logical and getNoteLong("oldtype") == prog.kw_can_find)
+         <action>...</action>
+      </rule>

After taking a closer look at the asts, the KW_* and RECORD_PHRASE are at the same depth so downPath(this, prog.kw_find, prog.kw_*) is never reached. I will also commit this.

I think downPath(this, prog.kw_find, prog.record_phrase) will cover all FIND statements. For example, a FIND FIRST ... will have the following AST:

   <ast text="statement" type="STATEMENT">
      <ast text="find" type="KW_FIND">
        <ast text="first" type="KW_FIRST"/>
        <ast text="record phrase" type="RECORD_PHRASE">

I committed 9078b/16010 to address #9078-101 and #9078-102.

Artur found a regression while converting a customer application, currently investigating.

Dănuț Filimon wrote:

Artur found a regression while converting a customer application, currently investigating.

I fixed the issue in 9078b/16011.

I looked at the fix in 16011. The changes are correct.

The 9078b/16012 seems OK.

Danut, does AdaptiveFind still emit properly? This is the FIND FIRST. REPEAT: FIND NEXT. END. pattern, if I'm not mistaken.

Constantin Asofiei wrote:

Danut, does AdaptiveFind still emit properly? This is the FIND FIRST. REPEAT: FIND NEXT. END. pattern, if I'm not mistaken.

Yes, it emits properly. AdaptiveFind is emitted a bit sooner.

Rebased 9078b to latest trunk/16028, the branch is now at revision 16037.

I've managed to fix the issue, I've also noticed that CAN-FINDs were ignored and AdaptiveFinds were also counted. I'll do a cleanup and a small test and commit my changes soon.

Dănuț Filimon wrote:

I've managed to fix the issue, I've also noticed that CAN-FINDs were ignored and AdaptiveFinds were also counted. I'll do a cleanup and a small test and commit my changes soon.

Committed the fix for #9078-113 and #9078-114 to 9078b/16038.

DO blocks are quite tricky, those blocks do not have an instvarid and in some cases I can get out of the expected block with a getAncestor call. I saw no other solution that using the controlid in those cases. Committed 9078b/16040.

I did a ChUI conversion and the project built with no issues, I noticed both the array and variable FindQuery types being used.

Rebased 9078b to latest trunk/16122, the branch is now at revision 16134.