FWD » Core Development » Conversion Tools

Use the following testcase:
a.p:

define temp-table tt no-undo
        field f1    as integer
        field f2    as integer
        field f3    as integer.

define temp-table tt no-undo
        field f11   as integer
        field f2    as integer
        field f3    as integer.

Full conversion generates two DMOs, Tt_1.java and Tt_2.java (field names differences trigger different structures). If I add some newlines in a.p and run incremental conversion, a new DMO is generated, Tt_3.java (which corresponds to Tt_1.java). The issue is that Tt_1.java should be reused and Tt_3.java should NOT be created.

I don't recall exactly when, but this was a known 'limitation' of incremental conversion, from few years back. I think this is a good time to find a fix for this and re-use the DMO.

It seems that pre_tmpTabNames_work is the one that doesn't contain Tt_1.java, not pre_tmpTabNames.

Make sure that we aren't duplicating work that was already done in 6083a.

I would go a different way: is OK to re-generate all DMOs, but the name should be preserved if the key matches. Maybe we need a global table (not per-file) with the DMO key and its name from the full conversion.

Constantin Asofiei wrote:

I would go a different way: is OK to re-generate all DMOs, but the name should be preserved if the key matches. Maybe we need a global table (not per-file) with the DMO key and its name from the full conversion.

Yes, I agree. From my understanding, this is what pre_tmpTabNames__work and tmpTabNames__work tables contain. They keep the interface names, respectively the implementation names, based on the AstKey.

This way, when we execute p2o_pre.xml and p2o.xml, we will generate new DMOs but we will use the names from the full conversion.

Danut has changes in this area in 6083a.

Greg Shah wrote:

Danut has changes in this area in 6083a.

I tested Danut's changes but the issue is not fixed.

   public static Set<String> clean(String file)
   {
      // go through tables and remove records for this specific file
      String[] exts = { "", ".ast", ".p2o", ".schema", ".dict" };
      Long[] fileIds = new Long[exts.length];
      AstManager mgr = AstManager.get();

      for (int i = 0; i < exts.length; i++)
      {
         if (exts[i].isEmpty())
         {
            continue;
         } 
      [...]

Does anyone know how could I generate this kind of hierarchy: Tt_1.java, Tt_1_1.java and Tt_1_2.java?

start.p:

def new shared temp-table tt no-undo
   field price as int
   field description as char.

run v1.p.

find first tt.
message tt.price.

v1.p:

def shared temp-table tt no-undo
   field price as int
   field description as char initial "desc".

create tt.
assign tt.price = 10.

My testcase generates Tt_1.java Tt_1_1.java for start.p and Tt_2.java for v1.p.

We should be converting everything dependent upon the Tt_1.java when it gets reconverted. If we don't include all programs that reference a compatible temp-table, then changes to that parent class could break the unmodified programs.

I don't know if we handle that properly in 6083a.

Because v1's definition has an additional initial "desc", FWD decides the tables are not compatible. I think this is probably a regression?

• the initial "desc" should not be taken into consideration for shared temp-tables, only when defining a new shared temp-table.

Details here: Data_Model_Objects

Greg Shah wrote:

We should be converting everything dependent upon the Tt_1.java when it gets reconverted.

Yes. I noticed that we should keep only the name of the super interfaces, in this case Tt_1.java which is stored in pre_tmpTabNames.

Keeping the sub-interfaces name generates errors due to how p2o.xml is designed:

               <action>tmpTabKey = create('com.goldencode.p2j.uast.AstKey', this, tmpTabCrit)</action>

               <rule>!tmpTabNames.containsKey(tmpTabKey)
               [...]                  
                  <rule on="false">true
                     <!-- log this access -->
                     <!-- <action>nextTTSuffixMap.containsKey(tt_interface)</action> -->
                     <action>oldPeer = tmpTabNodes.get(tmpTabKey)</action>
                     <rule>!oldPeer.isAnnotation("tt_suffix")

My next solution would be to turn this condition AND TABLE_NAME NOT IN ('PRE_TMPTABNAMES__WORK', 'TMPTABNAMES__WORK') into this AND TABLE_NAME NOT LIKE 'PRE_TMPTABNAMES__WORK'. But I want to test what's the impact of generating the second layer of interfaces (Tt1_1.java, Tt_1_2.java). That's why I needed a testcase with this hierarchy.

Ovidiu Maxiniuc wrote:

Because v1's definition has an additional initial "desc", FWD decides the tables are not compatible. I think this is probably a regression?

For some reason, the output is correct. The value of tt.price after running v1.p is 10. But I don't understand why a Tt_1_1 can be used as a Tt_2.

Indeed, that is a puzzle. Try to remove the build/ folder and reconvert/recompile the project again. Some classes may be dangling, remnant from a previous conversion.

Before doing this, please add the description to message tt.price. IIRC, even in v1.p it looks like its value should be "desc" it will actually be empty, since the create will use the definition from the new shared table. That is, if the code will not crash because of table mismatch.

I can confirm your observation that the _2 and subsequent interfaces are no longer created.

Therefore I studied a bit how the tmpTabCrit evolved in rules/schema/p2o_pre.xml, rules/schema/p2o.xml and rules/include/common-progress.rules. It has changed a bit since r11164. I think there is a regression since 20170901. I find it strange that we did not notice in the regression tests, especially ETF sets.

Review of 10639a/16189.
The code should work now. But I do not understand the usage of LIKE operator in this case (TABLE_NAME NOT LIKE 'PRE_TMPTABNAMES__WORK'). There is no pattern. It does the job, but it would be faster to use the equality test directly.

Ovidiu Maxiniuc wrote:

Review of 10639a/16189.
The code should work now. But I do not understand the usage of LIKE operator in this case (TABLE_NAME NOT LIKE 'PRE_TMPTABNAMES__WORK'). There is no pattern. It does the job, but it would be faster to use the equality test directly.

I changed that in 10639a/16190 ✅.

Octavian, please check #10536-27 - #10536-32 and #10536-38 - #10536-39.

Dănuț Filimon wrote:

Octavian, please check #10536-27 - #10536-32 and #10536-38 - #10536-39.

I included that patch in 10639a/16191. I tested the scenario and it works fine. Both DMOs are generated properly.

Alex: Please review.

Alex: Please review.

• I did not see any discussion upon what happens if you actually change the table. If you avoid clearing the PRE_TMPTABNAMES__WORK, does it mean that all (even unused) table schemas are still retained?
  • Please clarify how this happens.
  • Is this a potential memory leak?
• Of course, I think that changing a temp-table to match another one from another file won't make it share the same interface/multiplex, right? This might happen only if the other file is incrementally converted as well. This is also not happening in trunk, so it is fine not to assess now.
  • If I change a temp-table 100 times, would I end up with a new Tt_100, even though there are only 2 different Tt across the legacy code?

Overall, I think this is a quick fix that fixed #10639, but the technicality of incremental conversion is still fragile in terms of DMO merging. In other words, incrementally converting some files in a different order may generate different outputs (incrementally convert A, B and after C is different that incrementally converting C, A, B), considering temp-table definitions are changes.

Alexandru Lungu wrote:

I understand the change and it looks functionally right to me (especially after re-reading #10639). But I still have two concerns:

• I did not see any discussion upon what happens if you actually change the table. If you avoid clearing the PRE_TMPTABNAMES__WORK, does it mean that all (even unused) table schemas are still retained?
  • Please clarify how this happens.
  • Is this a potential memory leak?
• Of course, I think that changing a temp-table to match another one from another file won't make it share the same interface/multiplex, right? This might happen only if the other file is incrementally converted as well. This is also not happening in trunk, so it is fine not to assess now.
  • If I change a temp-table 100 times, would I end up with a new Tt_100, even though there are only 2 different Tt across the legacy code?

Indeed, this is correct. I tested those scenarios and the unused temporary table schemas are retained. This seems a consequence of the 10639a changes.

Alexandru Lungu wrote:

I understand the change and it looks functionally right to me (especially after re-reading #10639). But I still have two concerns:

• I did not see any discussion upon what happens if you actually change the table. If you avoid clearing the PRE_TMPTABNAMES__WORK, does it mean that all (even unused) table schemas are still retained?
  • Please clarify how this happens.
  • Is this a potential memory leak?
• Of course, I think that changing a temp-table to match another one from another file won't make it share the same interface/multiplex, right? This might happen only if the other file is incrementally converted as well. This is also not happening in trunk, so it is fine not to assess now.
  • If I change a temp-table 100 times, would I end up with a new Tt_100, even though there are only 2 different Tt across the legacy code?

As Alex mentioned, we can ignore the second point and create a separate task for that issue.
But the first point seems like an obstacle introduced by the current fix. This is what I noticed during analyzing the scenario described in the first point:

• After the full conversion of two file that use different schema temp-tables, I got Tt_1 and Tt_2 in PRE_TMPTABNAMES__WORK. As Alex already mentioned, if I change the schema of one file and run incremental conversion, I got a Tt_3 besides Tt_1 and Tt_2. Look at the content of PRE_TMPTABNAMES__WORK:

  FILEID	ASTID	POS	KEY	VALUE
  55834574848	55834574850	5	aced0001[...]	Tt_1
  60129542144	60129542146	6	aced0002[...]	Tt_2
  64424509440	64424509442	7	aced0003[...]	FwdEmbeddedWindow_1
  64424509440	64424509468	8	aced0004[...]	PageToolbar_1
  55834574848	55834574850	9	aced0005[...]	Tt_3

Shouldn't the pair (FILEID, ASTID) act like a primary key here?
In this case, it clearly doesn't. I think if we can fix that, the original issue can be fixed without any memory leak.

I think the key column should be part of the key in workInsertSql statement, to match the unique index.

     this.workInsertSql = "MERGE INTO " + workTable + " (fileId, astId, key, value) " +
                           "KEY (fileId, astId, key) VALUES (?, ?, ?, ?)";

Beside the evident hint from the unique index, skipping the key column would cause a single pair <key, value> to be possible for a <fileId, astId> entry. In your example, the key was aced000[...] - (apparently) the same for all entries, so it would work. But adding a different key for one of the existing <fileId, astId> will cause the existing one to be updated, even if the key is different.

I do not think this change will make any difference. It will only avoid UNIQUE INDEX KEY (fileId, astId, key) collisions. Since the conversion was executing without such incidents, this change will have no effect (except being more robust).

Returning to the issue you found, maybe the same-tree detection is not bulletproof? I think you might have found a flaw here.

Ovidiu Maxiniuc wrote:

In your example, the key was aced000[...] - (apparently) the same for all entries, so it would work.

I apologize for the confusion. In fact, all of the keys from #10639-31 are different. I edited the table to make this clear. Initially, I had to cut a lot of text from each key and all of those start with the same characters. That's why the key seemed to be the same, but it's not.

But adding a different key for one of the existing <fileId, astId> will cause the existing one to be updated, even if the key is different.

This is the expected behavior. We should not have two entries sharing the same key <fileId, astId. If that were the case, it would imply that a single file contains two different temporary tables defined by ASTs with identical IDs, which is fundamentally impossible. Therefore, I designed the process to overwrite those keys during incremental conversion to ensure consistency with the final output source code.

Ovidiu, is there something left to assess?

Ultimately, H2Map is a HashMap, which means it should associate values to keys. Here is the put() method:

   public Object put(Object key, Object value)
   {
      value = wrapValue(value);
      Object old = super.put(key, value);
      logChange(key, value);
      return unwrapValue(old);
   }

• during conversion we have funcRetTypes which is a collection meant to map the 4GL function names to their return types:
  

  <rule>funcRetTypes     = createString2StringMap("funcRetTypes")</rule>

• CommonAstSupport.java, createString2StringMap() delegates to ConversionData.createMap(String.class, String.class, name);
• from ConversionData.java we have a new H2Map(keyClass, valueClass, name, helper);

If we 'merge' using a key formed only from fileId, astId, this structure will be able to store the return type of a single function. It would be impossible to store information for function my-len return int (input character str) and function to-upper return character(input character str) from same source file. Since they belong to same file, both functions will share the fileId and AST root astId.

Ovidiu Maxiniuc wrote:

If we 'merge' using a key formed only from fileId, astId, this structure will be able to store the return type of a single function. It would be impossible to store information for function my-len return int (input character str) and function to-upper return character(input character str) from same source file. Since they belong to same file, both functions will share the fileId and AST root astId.

I'm not sure if that's correct. From my understanding, astId is the id of the associated source AST, not the AST root. Isn't that right?