FWD » Core Development » Conversion Tools

Initial report from #8938:

If the same query which is statically converted exists only as dynamic query, and this relation is not 'activated' during conversion of some other static query, then FWD has rules which at runtime establishes the relation between the tables, when executing the dynamic query.

This code which I assume is executed only for dynamic queries should be activated for static queries, too; and we can drop the relations from the DMO and the conversion rules which emit them as, considering the incremental conversion issues and the dynamic query support which is needed, they do not help.

From my brief analysis, the conversion rule responsible for adding the annotation is in dmo_common.rules:

<!-- many-to-one relation -->
            <variable name="refDb" type="java.lang.String" />
            <action>relAst = copy.getImmediateChild(data.many_to_one, null)</action>
            <while>relAst != null
               <!-- create the root relation annotation, if at least one relation found -->
               <rule>rootRelAst == null
                  <action>rootRelAst = tpl.graft("dmo_relations_annotation", null, javaRootId)</action>

                  <!-- position on the array initializer node -->
                  <action>rootRelAst = rootRelAst.firstChild</action>
               </rule>

               <!-- create the relation mapping -->
               <action>ref = getAst(#(long) relAst.getAnnotation("references"))</action>
               <action>refDb = #(String) relAst.getAnnotation("database")</action>
               <action>
                  primaryAst = tpl.graft("dmo_relation_annotation", null, rootRelAst,
                                         "rel_type", relAst.lookupTokenName(data.many_to_one),
                                         "name",     relAst.text,
                                         "database", refDb,
                                         "target",   #(String) ref.text)
               </action>
               <!-- if the relation is from another database we need to import it -->
               <rule>!dbName.equalsIgnoreCase(refDb)
                  <action>relCompAst = copy.getAncestor(-1, data.data_model)</action> <!--reuse-->
                  <action>
                     createJavaAst(java.kw_import, 
                                   sprintf("%s.%s.*", #(String) relCompAst.getAnnotation("package"), refDb),
                                   javaRootId)
                  </action>
               </rule>

               <!--loop through relation components-->
               <action>relComponentsAst = tpl.graft('dmo_relation_components', null, primaryAst)</action>

               <action>relCompAst = relAst.getImmediateChild(data.constr_col, null)</action>
               <while>relCompAst != null
                  <action>ref = getAst(#(long) relCompAst.getAnnotation("refid"))</action>

                  <!-- TODO: investigate this conditional. It was ported from old code but the logic behind
                             it's not very clear to me. Why is _meta database so special here? Most likely
                             there are other cases where this happens. -->
                  <rule>"_meta" == dbName
                     <action>
                        tpl.graft("dmo_relation_component2", null, relComponentsAst,
                                  "foreign_name", pKeyName,
                                  "local_name",   ref.text)
                     </action>
                     <action on="false">
                        tpl.graft("dmo_relation_component", null, relComponentsAst,
                                  "foreign_name", ref.text)
                     </action>
                  </rule>

                  <action>relCompAst = relAst.getImmediateChild(data.constr_col, relCompAst)</action>
               </while>

               <!-- advance to the next relation AST -->
               <action>relAst = copy.getImmediateChild(data.many_to_one, relAst)</action>
            </while>

Without this segment the relations annotation is dropped from full conversion.

• relies on the @Relations annotation to populate the relationship between tables
• builds this relationship at runtime for dynamic queries

• one with a static query which establishes the relationship during conversion
• one with a dynamic query which does the same, but at runtime

These both tests should work with current trunk. After that, we can work on removing the conversion changes (including the dmo_relations_annotation template and any others), the Relations annotation and enable the runtime to always compute and cache the relationship.

Isn't there a performance advantage to pre-calculating the relation (when we can)?

Greg Shah wrote:

Isn't there a performance advantage to pre-calculating the relation (when we can)?

The only change would be that the runtime will populate this relation cache only when it encounters a specific query. The runtime already needs to do this for the dynamic query case.

Greg Shah wrote:

Isn't there a performance advantage to pre-calculating the relation (when we can)?

There is one, but the gain might not be perceptible. In both cases the result is cached in DmoMeta and available using public RelationInfo getRelation(Class foreign). So, if the relation is not statically computed, after the first 'hit', the footprint is the same.

Andrei Plugaru wrote:

Managed to create a testcase with a dynamic query where the relations are created at runtime.

You know what is strange? This testscase does not work correctly for me. The prepare-query fails with a (dynamic) conversion error. I looked at it a bit but not enough to fully understand why the P2OLookup is empty for the temp-tables. Maybe it's something wrong with my FWD (I do not have a stock revision). At any rate, assuming the JAST is correctly constructed, the runtime should be identical to static conversion: the query should be constructed/initialized the same way, even if not visible in on-screen Java code.

Also, I find out that the relations are added at runtime using DmoMetadataManager.putDynamicRelation.

Yes, this is correct.

However, it seems interesting that it is also called for static queries when they are initialized and because the keys in the relations map are just Class objects with no custom hashCode/equals, the relations are actually added twice.

Indeed, the key for relations and inverseRelations maps are Class objects. They are guaranteed by JVM to be unique (maybe in a similar fashion as interned String object). Note that you cannot create Class objects, you get them from factories with will make sure you always get the same Object representing a specific class. As such, the equals is very strictly defined (c1 == c2). The hashcode() for it is probably not an issue; it is native, but I trust it is correctly defined as a hash function. Indeed, it would be nice to know some insights in order to fine-tune the map container, but this is just an optimisation.
Bottom line: I am positive, the relations maps function correctly.

As of right now, I think the relations that are added based on the annotations created at conversion are either not used or overridden.

I see the maps queried in the following stack:

      at com.goldencode.p2j.persist.orm.DmoMeta.getRelation(DmoMeta.java:1125)
      at com.goldencode.p2j.persist.AbstractJoin.<init>(AbstractJoin.java:155)
      at com.goldencode.p2j.persist.DynamicLegacyKeyJoin.<init>(DynamicLegacyKeyJoin.java:148)
      at com.goldencode.p2j.persist.PreselectQuery.processJoin(PreselectQuery.java:4718)
      at com.goldencode.p2j.persist.AdaptiveQuery.addComponent(AdaptiveQuery.java:1127)
      at com.goldencode.p2j.persist.AdaptiveQuery.initialize(AdaptiveQuery.java:735)
      at com.goldencode.p2j.persist.AdaptiveQuery.initialize(AdaptiveQuery.java:1)
      at com.goldencode.p2j.persist.PreselectQuery.initialize(PreselectQuery.java:1037)
      at com.goldencode.test.P7.lambda$1(P7.java:113)

I think your testcases are a bit too complicated. When UI is not required, please avoid using DISPLAY and FRAME s. Also, note that there are two kind of relations (direct and inverse), the second is really strange, but we encountered such constructs in customer code and FWD supports both of them:

// I kept the table definitions and population from your testcase
FOR EACH customers_d, EACH orders_d OF customers_d:
    MESSAGE "s -> :" customers_d.customer_id customers_d.customer_name orders_d.customer_id orders_d.order_amount.
END.

FOR EACH orders_d, EACH customers_d OF orders_d:
    MESSAGE "s <- :" customers_d.customer_id customers_d.customer_name orders_d.customer_id orders_d.order_amount.
END.

Ovidiu Maxiniuc wrote:

You know what is strange? This testscase does not work correctly for me. The prepare-query fails with a (dynamic) conversion error. I looked at it a bit but not enough to fully understand why the P2OLookup is empty for the temp-tables. Maybe it's something wrong with my FWD (I do not have a stock revision). At any rate, assuming the JAST is correctly constructed, the runtime should be identical to static conversion: the query should be constructed/initialized the same way, even if not visible in on-screen Java code.

I tested again the dynamic example and it converts and runs fine with my stock FWD.

Ovidiu Maxiniuc wrote:

Indeed, the key for relations and inverseRelations maps are Class objects. They are guaranteed by JVM to be unique (maybe in a similar fashion as interned String object). Note that you cannot create Class objects, you get them from factories with will make sure you always get the same Object representing a specific class. As such, the equals is very strictly defined (c1 == c2). The hashcode() for it is probably not an issue; it is native, but I trust it is correctly defined as a hash function. Indeed, it would be nice to know some insights in order to fine-tune the map container, but this is just an optimisation.
Bottom line: I am positive, the relations maps function correctly.

I was, indeed, wrong that the same Class objects are added twice. After I tested the static example once again, I realized that based on the annotations, the DMO sub-interface is added as the key. However, when processing the query, the DMO super-interface was added as key. I have done this investigation in DmoMeta.putRelation. After processRelations call this was the relations map state {interface com.goldencode.dataset.dmo._temp.Customers_1_1=com.goldencode.p2j.persist.orm.RelationInfo@12e40de7}.
After the dynamic relation is added, {interface com.goldencode.dataset.dmo._temp.Customers_1_1=com.goldencode.p2j.persist.orm.RelationInfo@12e40de7, interface com.goldencode.dataset.dmo._temp.Customers_1=com.goldencode.p2j.persist.orm.RelationInfo@46cb7784}.
My question on this part is whether this behaviour for the static query was really intended and both relations are necessary?

I will also want to test what happens when inverse relations are used.

Andrei Plugaru wrote:

My question on this part is whether this behaviour for the static query was really intended and both relations are necessary?

A good observation. It looks like the relations read from DMO annotations are incorrect for the temp-tables (they map using incorrect DMO class key, for example target = CustomersD_1_1.class instead of target = CustomersD_1.class). As result, these relations will not be found when looked up so the runtime detection is used, anyway, for temp tables. The annotation of permanent tables are correct.

Well, this is, yet, another reason to drop generation of these annotations.

I will also want to test what happens when inverse relations are used.

You need to replace:

qh:SET-BUFFERS(BUFFER customers_d:HANDLE, BUFFER orders_d:HANDLE).
qh:QUERY-PREPARE("FOR EACH customers_d, EACH orders_d OF customers_d").

qh:SET-BUFFERS(BUFFER orders_d:HANDLE, BUFFER customers_d:HANDLE).
qh:QUERY-PREPARE("FOR EACH orders_d, EACH customers_d OF orders_d").

Note:
There is a 2nd type of relations (known as parent-recid relations) but they are already exclusively detected at runtime, so there is nothing to do in this task.

I have done some more investigation on relations for persistent tables. Based on that I confirmed that the relations created based on the annotations are correct as Ovidiu stated in the last note. However, even if they are not created based on the annotations, they will still be created at runtime. To be more precise, in AbstractJoin constructor, it is, first, verified whether the relation exist and if that is not the case, it will be added. I have also tested inverse relations, like it was mentioned in #8967-12, and they are still added.
For temporary tables, I have already confirmed they are added at query initialization, without needing the @Relation annotation, as stated in #8967-11, #8967-13.

In conclusion, it would seem that just removing the @Relation annotation shouldn't make any difference functionally, as all the relations should be added when the query is initialized. In order to test that, I have also tried to manually delete the annotation on some converted code, compile it again, and, indeed the relations were added dynamically.

Ovidiu/Constantin, when any of you is available, can you please confirm my conclusion is valid if there are any edge cases when it doesn't hold?
If that is not the case, I would think that just deleting the rule that creates the annotation(which Serban already found and posted in #8967-2) is safe and should't produce any issues.

Andrei,
I think your conclusion is solid. As noted in last phrase form note 8, please proceed to drop the conversion support for both AST processing and TRPL template.

I have committed my changes to 8967a/rev 15447

• remove the templates for relation and relations annotation in java_templates.tpl
• remove the rule for the many-to-one relation in dmo_common.rules
• drop the processing of the @Relation annotation in DmoMeta.java

However, I have some issue testing with this new branch. I got this error in server.log:

com.goldencode.p2j.cfg.ConfigurationException:  Initialization failure
        at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2610)
        at com.goldencode.p2j.main.StandardServer.bootstrap(StandardServer.java:1270)
        at com.goldencode.p2j.main.ServerDriver.start(ServerDriver.java:526)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:522)
        at com.goldencode.p2j.main.ServerDriver.process(ServerDriver.java:228)
        at com.goldencode.p2j.main.ServerDriver.main(ServerDriver.java:955)
Caused by: java.lang.RuntimeException: Error initializing persistence services
        at com.goldencode.p2j.persist.Persistence.initializeTenant(Persistence.java:3533)
        at com.goldencode.p2j.persist.Persistence.<init>(Persistence.java:986)
        at com.goldencode.p2j.persist.PersistenceFactory.getInstance(PersistenceFactory.java:190)
        at com.goldencode.p2j.persist.meta.LockTableUpdater.<init>(LockTableUpdater.java:194)
        at com.goldencode.p2j.persist.DatabaseManager.initDb(DatabaseManager.java:1679)
        at com.goldencode.p2j.persist.DatabaseManager.initialize(DatabaseManager.java:1509)
        at com.goldencode.p2j.persist.Persistence.initialize(Persistence.java:1039)
        at com.goldencode.p2j.main.StandardServer$6.initialize(StandardServer.java:1522)
        at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2606)
        ... 5 more
Caused by: java.lang.NullPointerException: Cannot invoke "java.util.Queue.add(Object)" because "com.goldencode.p2j.persist.orm.SessionFactory$SessionCloseThread.factories" is null
        at com.goldencode.p2j.persist.orm.SessionFactory$SessionCloseThread.register(SessionFactory.java:315)
        at com.goldencode.p2j.persist.orm.SessionFactory.<init>(SessionFactory.java:118)
        at com.goldencode.p2j.persist.Persistence$Context.<init>(Persistence.java:4635)
        at com.goldencode.p2j.persist.Persistence$1.initialValue(Persistence.java:807)
        at com.goldencode.p2j.persist.Persistence$1.initialValue(Persistence.java:804)
        at com.goldencode.p2j.security.ContextLocal.getImpl(ContextLocal.java:556)
        at com.goldencode.p2j.security.ContextLocal.get(ContextLocal.java:460)
        at com.goldencode.p2j.persist.Persistence.message(Persistence.java:3413)
        at com.goldencode.p2j.persist.Persistence.createLockManager(Persistence.java:3585)
        at com.goldencode.p2j.persist.Persistence.initializeTenant(Persistence.java:3512)
        ... 13 more

Managed to overcome the error posted in #8967-19. The root cause is the call to message method from createLockManager in Persistence class. As a workaround, I changed the logging level to INFO.

Andrei Plugaru wrote:

Managed to overcome the error posted in #8967-19. The root cause is the call to message method from createLockManager in Persistence class. As a workaround, I changed the logging level to INFO.

Thank you for investigating this. I will add the fix in my current working branch.

Andrei Plugaru wrote:

I have committed my changes to 8967a/rev 15447

• dmo_relation_components, dmo_relation_component2, and dmo_relation_component templates seem unused. Please double check this and remove if the case. This is low risk.
• the code removes processing the many_to_one nodes, but they are still computed. If we avoid collecting this information (we do not use any more) we can gain a bit of performance at conversion. Please see SchemaWorker.joins and related code. The hot point is SchemaWorker$SchemaSupport.recordJoin() method. I tried disabling it, but I got a lot of differences in intermediary artifacts for my testcases. A bit difficult to compare, but is seems that only relations nodes are removed and the other annotations are reordered (?). The resulting code seem fine in my very simple testcases, but it might not be in case of larger applications where the relationship between the table are more complicated. This involves a higher risk, and requires testing the conversion of the applications to be sure there are no regression.

Addressed the first part of the review in 8967a/rev 15448.

About the second part, I would have some concerns about disabling recordJoin method. In this method, joins map is populated and that is used, for example, in getForeignKey public method. At a quick search, I found this method used in index_selection.rules. Based on that, I would think that the static detected relations may be used in other places.

Ovidiu, I would propose that, until we decide whether we should update SchemaWorker or not, I start the conversion of a large customer application in order to validate the current changes. Furtheremore, the main purpose of this task is done, and if we are able to gain a bit of performance at conversion, we can make these changes a little bit later, after the current changes are delivered.

The conversion of a large customer application went fine. I will proceed to testing.

Ran all the tests provided by a customer and no regressions have been found.

8967a has been merged to trunk and ported to 7156b.