TRPL

There is an early version of a multi-threaded pattern engine rewrite in com.goldencode.trpl.*. It is fairly clean but is a bit naive. It is also unfinished. For example, it has no implementation of TRPL itself. It was being tested with "rules" implemented directly as Java objects that implement a specific interface (TreeWalker).

Constantin: I know you started work on this some time ago. What task had those results and are there any other notes we should make regarding this effort?

Thoughts on this?

Greg, I can't find an exact task for this - I think the discussion was spread-out for runtime query conversion, incremental conversion, adding OO support.

• for OO conversion
  • I recall we order the .cls files via an hierarcy-dependency. This should still be honored.
  • a lock on the .cls-file level - i.e. if a dependency is parsed on another thread, then the requester for that dependency should wait for it to be fully parsed
• for incremental conversion: the access to the 'cvtdb' needs to be made thread-safe
• in TRPL you can also have 'global' variables for a pipeline

Greg Shah wrote:

1. The core PatternEngine itself (and all of its dependent code such as the SymbolResolver, XML loading, expression parsing and compiler) needs to be reworked to be multithreaded. That is the purpose of this task. The changes will involve implementing the threading model itself, implementing the proper protection for all shared data internal to TRPL itself and handling whatever thread IPC facilities as needed to coordinate the threaded processing.

After successfully making changes to eliminate static data and singleton patterns from TRPL (see #1752), I believe we can now focus on the first point outlined below. This will help us more easily identify potential synchronization conflicts and dependencies between system components that need protection in a multithreaded environment. Additionally, it will allow us to uncover all shared data within TRPL worker classes and to see if there are some other classes which need to have a context-local implementation. I understand that PatternEngine should be thread-safe, but where exactly should the thread work be split? Perhaps we can group the processTrees calls based on their dependencies (such as required order or specific annotations).

I understand that PatternEngine should be thread-safe, but where exactly should the thread work be split? Perhaps we can group the processTrees calls based on their dependencies (such as required order or specific annotations).

Yes, the PatternEngine itself will implement the multithreading which means that any caller (e.g. ConversionDriver) will not overlap PatternEngine invocations. In other words, for a multi-phased process like ConversionDriver, each phase will complete (using multithreading) before the next phase begins. This has a big advantage of being easier to reason about. Code in phase 2 can rely upon things done in phase 1.

I would expect us to implement the core threading approach in PatternEngine.run() which is the "main loop" that processes a given execution of the PatternEngine.

OK. So, processing each AST (or walk call graph) is the job that needs to be divided between threads, right?

         // Process each stored AST or walk call graph, depending on the mode of operation.
         Iterator<String> iter = targetPaths.iterator();
         while (iter.hasNext())
         {
            String path = iter.next();

            config.withFileProfile(path);

            if (graph != null)
            {
               Aast ast = AstManager.get().loadTree(path);
               long id = ast.getId();
               Vertex node = nodeFilter.acceptNode(id);
               if (node != null)
               {
                  walkGraph(node);
               }
            }
            else
            {
               processAst(path);
            }
         }

Octavian Adrian Gavril wrote:

OK. So, processing each AST (or walk call graph) is the job that needs to be divided between threads, right?

[...]

This way, the order of the phases is the same and the conversion architecture is not changed. The only difference will be that each phase processes the same amount of files faster by using more threads.

Yes, but there is also the 2nd phase:

         // Process each in-memory AST
         for (int i = 0; i < targetAsts.size(); i++)
         {
            Aast targetAst = targetAsts.get(i);
            processAst(targetAst);
         }

I think we probably should eliminate the duality of building/processing the targetPaths and separately building/processing the targetAsts. The PatternEngine itself should not do any file system listing. The list of ASTs should be explicitly provided by the caller.

Also, we should either unify run() and run(String) or we must implement multithreading in both.

Octavian Adrian Gavril wrote:

Octavian Adrian Gavril wrote:

OK. So, processing each AST (or walk call graph) is the job that needs to be divided between threads, right?

[...]

This way, the order of the phases is the same and the conversion architecture is not changed. The only difference will be that each phase processes the same amount of files faster by using more threads.

Exactly right.

Octavian Adrian Gavril wrote:

Do you think it would be better to safely share the PatternEngine instance and the resolver between threads, or should each thread have its own isolated instances?

The PatternEngine and SymbolResolver instances must be shared, so they will need to be made thread safe.

Some context-local implementations are a bit problematic in scenarios where threads need to share the same data. An example would be using SymbolResolver.loadConvertedClass. One phase might load that class, but only in its own context, and in the other phase that class is not loaded in the current context. I understand that this context-local is needed for runtime query conversion support.

Octavian Adrian Gavril wrote:

Some context-local implementations are a bit problematic in scenarios where threads need to share the same data. An example would be using SymbolResolver.loadConvertedClass. One phase might load that class, but only in its own context, and in the other phase that class is not loaded in the current context. I understand that this context-local is needed for runtime query conversion support.

Anything that is shared data is not context local and vice versa. The context-local approach can only be used for data that is specific to a "single thread". It is just used in FWD instead of thread local, but it is the conceptual equivalent.

Do you think the resolver instance should not be shared? I'm concerned that fields such as ruleScope, targetRootAst, currentCondition and others might cause conflicts when multiple rules interleave and operate on the same resolver instance. Currently, these fields are synchronized at the usage level—through setters/getters or per instruction—rather than at the context level as a whole.

To be clear, we are taling about AstSymbolResolver here. I think some data/methods need to be shared and some data will need to be context-local.

engine, workers, resolvingContainers and astMap seem like shared state that needs to be carefully synchronized. But view, sourceAst, copyAst, nextChildIndex and others like the ones you note above are context-local for sure.

I’ve committed changes to 1752a/15889. In this version, I prepared the threads to work correctly for a full conversion of the hotel_gui application. I also extended context-local functionality in AstSymbolResolver.

   /**
    * Context local work area.
    */
   private static class WorkArea
   {
      /** Context-local instance of this class */
      private AstSymbolResolver instance;

      /** Set of filtered results which define a view on the original AST */
      private Set<Aast> view = null;

      /** Rule container which is currently in scope */
      private RuleContainer ruleScope = null;

      /** Read-only source AST currently being processed */
      private Aast sourceAst = null;

      /** Copy of source AST which may be modified by actions */
      private Aast copyAst = null;

      /** Flag to indicate walking should cease after the current AST node */
      private boolean endWalk = false;

      /** Map of ids to ASTs */
      private Map<Long, Aast> astMap = new HashMap<>();

      /** Text of conditional expression currently under test */
      private String currentCondition = null;

      /** Root node of target (converted code) AST hierarchy */
      private Aast targetRootAst = null;

      /** Tracks the next child index during walking. */
      private int nextChildIndex = -1;
   }

I added astMap since AstSymbolResolver.cleanup is used by each thread. I'm continuing to fully integrate these context-related changes with concurrent execution.

I added astMap since AstSymbolResolver.cleanup is used by each thread. I'm continuing to fully integrate these context-related changes with concurrent execution.

This will be a tricky area. I think we will find arbitrary usage of things like getAst() to access AST nodes. I think we will likely use that heavily in the OO support code. By arbitrary, I mean we won't be sure it is done on the same thread that holds the lock for the entire AST.

On the other hand, we do clear and repopulate the map every time we shift from copy to this. That is indeed a per-thread concept.

If we find cross thread access, then we may want to find ways to share the state needed without cross-thread direct access to AST nodes.

I have a scenario where both threads are applying these rules, but on different files:

      <rule>type == prog.assign
         <action>ref = copy.getFirstChild()</action>
         <rule>ref.type == prog.colon
            <!-- the last element in the chain will always be the second child of the topmost COLON node -->
            <action>ref = ref.getChildAt(1)</action>

            <!-- except if it is a DB_REF_NON_STATIC, which is probably an incorrect tree build;
                 TODO: resolve this -->
            <while>ref.type == prog.db_ref_non_static 
               <action>ref = ref.nextSibling</action>
            </while>

            <rule>evalLib("read_only_attribute", ref)
               <action>
                  printfln("READ_ONLY_WARNING: REWRITING attribute assignment as string at %s line %05d column %05d",
                           file,
                           line,
                           this.getColumn())
               </action>

               <!-- inject the name of the READ/ONLY attribute as a new 2nd child character expression -->
               <action>pref = createProgressAst(prog.expression, copy, 1)</action>
               <action>pref.putAnnotation("read-only attribute helper", true)</action>
               <action>pref = createProgressAst(prog.string, pref)</action>
               <action>pref.text = sprintf('"%s"', ref.text)</action>
               <action>pref.putAnnotation("no_wrap", true)</action>
               <!-- this way, the actual assigned expression is shifted to 3rd position -->
            </rule>
         </rule>
      </rule> 

ref = ref.getChildAt(1)
     [java]           ^  { Cannot invoke "com.goldencode.ast.Aast.getChildAt(int)" because the return value of "com.goldencode.expr.CompiledExpression.getVar(int)" is null 

try
      {
         initialize(profile);

         applyGlobal(RULE_INIT);

         CountDownLatch latch = new CountDownLatch(2);

         Configuration config = Configuration.getInstance();

         AstProcessorJob job1 = new AstProcessorJob(targetPaths, targetAsts, config, graph, nodeFilter,this, resolver, 0, latch, JavaPatternWorker.getLocal(), AstManager.get(), SymbolResolver.locate());
         AstProcessorJob job2 = new AstProcessorJob(targetPaths, targetAsts, config, graph, nodeFilter,this, resolver, 1, latch, JavaPatternWorker.getLocal(), AstManager.get(), SymbolResolver.locate());

         threads.get(0).queue.add(job1);
         threads.get(1).queue.add(job2);

         latch.await();

Do you think the initialization and execution of init/post rules should happen on a per-thread basis? If so, should PatternEngine have a thread-local implementation for fields like namespaces, includes, and other components related to rule loading?

Do you think the initialization and execution of init/post rules should happen on a per-thread basis?

Global init/post rules needs to be on a single thread, before (init) and after (term) all the multithraded rule processing occurs.

Non-Global init/post rules must be per-thread.

If so, should PatternEngine have a thread-local implementation for fields like namespaces, includes, and other components related to rule loading?

Hmmm. I think rules must have per-thread instances. I wonder what the memory impact is for that.

Greg Shah wrote:

Hmmm. I think rules must have per-thread instances. I wonder what the memory impact is for that.

I tested this today. The rules attribute gets populated during the initialization of the PatternEngine. I moved rules into a context-local environment, but I encountered issues related to inheriting rules from the main thread into other threads. Because context.get().rules is null in different threads by default, this caused problems with recursive context access while copying instances of RuleContainer (such as RuleSet). I also attempted to populate rules within each thread during the initialization of PatternEngine, but that still wasn’t sufficient:"

            lastRule = lastRuleContainer.addRule(expr, type, fileName, getLineNumber());
            int ttype = type;
            PatternEngine.threads.get(0).queue.add(() -> lastRuleContainer.addRule(expr, ttype, fileName, getLineNumber()));
            PatternEngine.threads.get(1).queue.add(() -> lastRuleContainer.addRule(expr, ttype, fileName, getLineNumber()));

I'm exploring a different approach. Perhaps we should consider running each thread with its own copy of the PatternEngine?

Are we instantiating multiple copies of the expression classes (one per thread)? Make sure the containers are initialized with nothing shared between the threads.

Perhaps we should consider running each thread with its own copy of the PatternEngine?

I understand the appeal. Each PatternEngine would be single threaded. We would only have to coordinate access to those resources which must be access across different PatternEngine instances.

The core problem with this idea is that there essentially would be very little sharing object instances and state between these instances. We would greatly inflate memory with duplicates of the entire infrastructure (workers, state...). I don't think that is a good idea. I think it would be very inefficent and we would have very little ability to improve that efficiency.

It also makes management of the overall process very unclear. The global init/post processing is harder to handle and the overall processing is no longer in the PatternEngine itself. I don't love that.

I realize that making the PatternEngine itself (and all the workers) multithreaded is harder to do that the "one PatternEngine per thread" idea. But I don't think that the "one PatternEngine per thread" idea get us to our objective. Please don't go this direction.

Greg Shah wrote:

Are we instantiating multiple copies of the expression classes (one per thread)? Make sure the containers are initialized with nothing shared between the threads.

Expressions are initialized in the Rule constructor when ConfigLoader.load is called, meaning all instances are currently shared. The same applies to containers like RuleSet, which are used by both threads since the engine is shared and rules are not thread-specific.

   public Rule(String infix, int type, String file, int line)
   {
      condition = AstSymbolResolver.createExpression(infix);
      this.type = type;
      this.file = file;
      this.line = line;
   }

I realize that making the PatternEngine itself (and all the workers) multithreaded is harder to do that the "one PatternEngine per thread" idea. But I don't think that the "one PatternEngine per thread" idea get us to our objective. Please don't go this direction.

Understood. It appears that making all rules, rulesets, and expressions thread-specific is mandatory, so I’ll proceed with that approach.

As discussed in today's IDE meeting, as a default approach I would expect that functions and variables are both strongly scoped and are part of a thread local (OK, context local for us) instance so that the state is not shared across threads by default.

About the strong scoping, we have cases where we define state in enclosing scopes and directly access that state by name in nested scopes. For example, in annotations.xml we define variables like cls2super and then the var is used by name in cross_namespace_conflicts.rules. I think we should move away from that pattern. It is confusing and potentially a source of bugs. But to do so will require that we implement some replacement.

Variable and function scoping that is local to the context/thread is also very important by default. But I can also see that we might have use cases where thread-safe shared state is needed. I am open to implementing a version of variables that are thread-safe and automatically shared across threads, but only if implementing the idea makes the TRPL rules a lot cleaner and simpler. To implement this idea, I would thing we could add a thread-safe attribute to the variable definition. The resulting variable would then have an instance that was shared across threads and an implementation that uses some form of mutex or locking or synchronization to implement thread safety.

My initial approach to make rules, rulesets, and expressions thread-specific missed a key detail: only the main thread will be responsible for registering variables and functions within its ruleset as scope. They were thread-specific, but they just weren't complete.

   public RuleSet addRuleSet()
   {
      if (rules.get() == null)
      {
         rules.set(new ArrayList<>(1));
      }

      RuleSet ruleSet = new RuleSet(this);
      rules.get().add(ruleSet);

      PatternEngine.threads.get(0).queue.add(() -> {
         if (rules.get() == null)
         {
            rules.set(new ArrayList<>(1));
         }

         rules.get().add(ruleSet);
      });
      PatternEngine.threads.get(1).queue.add(() -> {
         if (rules.get() == null)
         {
            rules.set(new ArrayList<>(1));
         }

         rules.get().add(ruleSet);
      });

      return ruleSet;
   }

I tried to fixed that by deeply copy the entire rules collection in each thread:

   private void extractRules(RuleContainer container)
   {
      if (container.ruleList() != null)
      {
         ArrayList<RuleListElement> containerRules = container.ruleList();

         ArrayList<RuleListElement> copiedRules = containerRules.stream()
                 .map(e -> {
                    if (e instanceof RuleSet) {
                       return ((RuleSet) e).copy();
                    }
                    return e;
                 })
                 .collect(Collectors.toCollection(ArrayList::new));

         rules.putIfAbsent(index++, copiedRules);

         for (RuleListElement element : containerRules)
         {
            if (element instanceof RuleContainer)
            {
               extractRules((RuleContainer) element);
            }
         }
      }
      else
      {
         rules.putIfAbsent(index++, null);
      }
   }

   private void computeRules(RuleContainer container)
   {
      ArrayList<RuleListElement> extractedRuleList = rules.get(index++);

      container.setRules(extractedRuleList);

      ArrayList<RuleListElement> currentContainerRules = container.ruleList();

      if (currentContainerRules != null)
      {
         for (RuleListElement element : currentContainerRules) {
            if (element instanceof RuleContainer) {
               computeRules((RuleContainer) element);
            }
         }
      }
   }

But the new problem was that setting the context local of the same engine instance has effect for all threads. So in the end, both threads have the same rules.

  private ContextLocal<ArrayList<RuleListElement>> rules = new ContextLocal<>();

It seems that we need to include those variables/functions related registrations in each thread context.

I've committed changes in 1752a/15890 that make rules thread-specific (provisional implementation) and improve locking in AstSymbolResolver. I removed synchronized modifiers to enable true concurrency, but this caused some new errors with workers (as expected). Should we incorporate new changes related to #1753 in the next revision to help resolve this?

Should we incorporate new changes related to #1753 in the next revision to help resolve this?

Yes. I only split it into separate tasks to make the work more obvious. The changes will logically be best in the same branch.

I see that there are .xml file with a 'main' <rule-set> where init/post rules are included executing operations like initialization/cleaning of data. For example schema/annotations.xml:

<cfg>

   <!-- register worker objects -->
   <worker class="com.goldencode.p2j.uast.ProgressPatternWorker" namespace="prog" />
   <worker class="com.goldencode.p2j.schema.SchemaWorker"        namespace="schema" />

   [...]

   <!-- main processing (once per AST) -->
   <rule-set input="tree">

      [...]

      <init-rules>
         <!-- load schema data associated with this AST -->
         <rule>schema.initializeDictionary(file)</rule>
      </init-rules>

         // Run ruleset's init rules.
         ruleSet.apply(resolver, RULE_INIT);

I was wondering how we should handle similar scenarios, having shared workers. These rules need to be executed by the main thread?

Octavian Adrian Gavril wrote:

I see that there are .xml file with a 'main' <rule-set> where init/post rules are included executing operations like initialization/cleaning of data. For example schema/annotations.xml:
[...]
applyGlobal(RULE_INIT); call from PatternEngine.run(profile) is executed by the main thread. But this will execute just <init-rules> direct tags. So, schema.initializeDictionary(file) will be executed by each thread in PatternEngine.apply:

[...]

I was wondering how we should handle similar scenarios, having shared workers. These rules need to be executed by the main thread?

Actually, I understand that data is associated with each AST... I think that initialization should be thread-specific.

Correct

I tried to make ScopeState full thread-specific for each RuleContainer, including PatternEngine. But this was a bit problematic with 'global' variables defined in the .xml file. So, I tried to implement the idea that you described in #3211-27, making Variable thread-safe. Now I have some issues with variables used as counters and some errors are thrown: java.lang.IndexOutOfBoundsException: Index 1 out of bounds for length 1. Here is an example:

   <variable name="peers"              type="java.util.List" />
   [...]
   <variable name="denormCounter1"     type="java.lang.Integer" />
   [...]
            <action>denormCounter1 = 1</action>
            <action>denormSize = peers.size()</action>
            <while>denormCounter1 &lt;= denormSize
               <action>peer = peers.get(denormCounter1 - 1)</action>

               [...]
               <action>denormCounter1 = denormCounter1 + 1</action>
            </while>

I added the current version of synchronized profile variables scoping as revision 15892 in branch 1752a. However, I added some 'hardcoded' locking in .rules file to force threads to wait for each other when global variables are involved in blocks such as <while>.

I believe some profiles require adjustments to their variable usages. When comparing the output ASTs during conversion, specifically until P2O Generation (database schema files), I've found that moving all global variables from gaps/gap_analysis_marking.xml into the main <rule-set> tag results in identical generated files from multithreaded conversion compared to the original trunk conversion. This comparison was done using meld, ignoring id values from .schema files and other randomly generated values. I think sometimes we should treat this global scope as if it were local scope. However, I'm concerned about the memory impact. What are your thoughts?

I think the global scope variables should be local/thread-specific as well. If any of that is truly shared state, we can do something special (expose it as synchronized data in a worker or create a thread-safe variable. But let's try to move all of it to local context first.

However, I'm concerned about the memory impact.

It is probably just the price we have to pay for multithreading. If it is something that is too much, we will work on optimizing it as a future step.

I've committed some changes into 1752a/15893. I reverted the previous implementation of Variable class being thread-safe and added context-local implementation of variables scoping. I reverted 'hardcoded' locking from .rules files as well. I retested the hotel_gui conversion until P2O post-processing (database schema files) (exclusively). Mostly, there are no big differences. Three .p2o files have one extra annotation that is not attached in the original conversion.
I noticed a big difference about hotel/standard.p2o/schema files. All natural joins are registered by each thread and there are multiple operations over the database which are initiated by each thread. So I think that could be the case you mentioned above in #3211-37, where some shared variables are needed.

• one method call from a java file has one less parameter;
• adm_windows.xml has two extra ] character. This is cause by <post-rules> from adm/adm_windows.xml executed by each thread;
• name_map.xml is bigger compared with the original sources. Probably, all thread are writing same paths in this file.

In this version, ScopeState class in Scope.java is implemented as thread-specific, which is making the overall process heavier than the original conversion. My focus remains on keeping the context-local implementation as strict as possible. I'm also considering whether rules and rulesets should be thread-specific if variable scoping is handled in the same way. I'll be experimenting to explore this further.

• made some worker method calls from <post-rules to be executed by the main thread only. This way, adm_windows.xml, name_map.xml, hotel/standard.p2o/schema files are fixed;
• ConversionData.helper is now shared. This change gives to each thread the content of h2 data structures accumulated from previous profiles (global variables with the same name).
• output sources are identical;
• artifacts from root project are identical;
• abl folders are identical. That difference about "_false" suffix is caused by p2j different revisions. The other difference related to an annotation is caused by the different order of processing temp-tables asts.

made some worker method calls from <post-rules to be executed by the main thread only. This way, adm_windows.xml, name_map.xml, hotel/standard.p2o/schema files are fixed;

Are you talking about global post rules? Global init and post rules should only ever run on the main thread.

Greg Shah wrote:

Are you talking about global post rules? Global init and post rules should only ever run on the main thread.

Yes. I agree, but global variables are initialized in <init-rules>.

Octavian Adrian Gavril wrote:

Greg Shah wrote:

Are you talking about global post rules? Global init and post rules should only ever run on the main thread.

Yes. I agree, but global variables are initialized in <init-rules>.

You mean global in scope but not shared?

This suggests that we need a different mechanism for those so that the init rules are not used for that purpose.

Greg Shah wrote:

You mean global in scope but not shared?

I'm referring to global variable from each .xml file that we discussed about in #3211-37.

Octavian Adrian Gavril wrote:

Greg Shah wrote:

You mean global in scope but not shared?

I'm referring to global variable from each .xml file that we discussed about in #3211-37.

Right. That is what I mean by "global in scope but not shared".

Those need to be initialized outside of the global init-rules, so that the global init-rules (and global post rules) are only ever done on the main thread.

I refactored the variables scope context-local implementation. ScopeState is not used as thread-specific instance anymore. I move this approach into the Variable.value field. This is implemented using a threadSafe flag which indicates if the current variable is shared. This way, if threadSafe is true, value will be an Object and processed using locking/synchronization. Otherwise, the value will be ContextLocal<Object> and the variable will be thread-specific:

   Variable(String name, Class type, Initializer initializer, boolean readOnly, boolean threadSafe)
   {
      [...]

      this.threadSafe = threadSafe;

      if (!threadSafe)
      {
         this.value = new ContextLocal<>();
      }

      // Set the value of the variable using the initializer. If no
      // initializer is defined, set it to null.
      reset();
   }
   [...]
      public void setValue(Object value)
   {
      [...]

      if (threadSafe)
      {
         synchronized (this)
         {
            this.value = value;
         }
      }
      else
      {
         ((ContextLocal<Object>) this.value).set(value);
      }
   }

I integrated this flag in TRPL as well:

   <variable name="ooVarTempIdx"     type="java.util.Map" threadSafe="true" /> <!-- String, Long -->
   [...]

   <init-rules>
      <rule>ooVarTempIdx     = createString2LongMap("ooVarTempIdx")</rule>
      [...]
   </init-rules>

I've updated all .xml files. hotel_gui converted successfully, but the sources aren't compiling. I still need to resolve the current differences in the source code.

OK, that seems reasonable. The behavior of global init and post is meant to be executed once, not once per thread. But the non-thread-safe version of a variable will need to be initialized per thread. Do you copy the value of each var after global init to make it the initialValue()?

Greg Shah wrote:

OK, that seems reasonable. The behavior of global init and post is meant to be executed once, not once per thread. But the non-thread-safe version of a variable will need to be initialized per thread. Do you copy the value of each var after global init to make it the initialValue()?

I considered to make all variables that are used in init-rules/post-rules to be shared and thread-safe. So, these will be initialized once.

Actually, I found two cases where global variables are initialized in init-rules but those shouldn't be shared. To avoid conflicts across thread I move the initialization into the variable declaration:

-   <variable name="brsMap"    type="java.util.Map" />
[...]
+   <variable name="brsMap"    init="#(java.util.Map) create('java.util.LinkedHashMap')" />
[...]

    <!-- global initialization (once per pipeline) -->
    <init-rules>
@@ -585,8 +585,6 @@
       <rule>uiStrings = createStringSet('uiStrings')</rule>
       <rule>orphanFrames = createStringSet("orphanFrames")</rule>

-      <rule>brsMap = #(java.util.Map) create('java.util.LinkedHashMap')</rule>
-

This way I fixed the last differences and now the hotel_gui converts and compiles successfully. abl, cvt and src folders are identical.

I've committed the last changes into 1752a/15896.

To avoid conflicts across thread I move the initialization into the variable declaration:

This should not be required. The global init/term MUST run on the main thread. That is the contract. If we have a non-thread-safe var, then we must implement the initialValue() to copy the "prototype" instance that is initialized on that main thread. That "prototype" should never be touched because the main thread should be waiting for the other threads to complete and doesn't need to be running any logic itself.

Octavian, just as a side note, Java 21 finally introduced virtual or 'green' threads, the advantages over physical threads are:

• Higher scalability - Virtual threads can scale to millions, vs. thousands for platform threads.
• Lower memory usage - Virtual threads use a few KB stack vs. ~1MB for platform threads.
• Less OS overhead - Managed by the JVM, not the OS; fewer context switches.
• Simpler concurrency - Enables blocking code without blocking OS threads.
• Faster thread creation - Virtual threads are cheaper and faster to start and stop.

Here is the documentation: https://docs.oracle.com/en/java/javase/21/core/virtual-threads.html#GUID-DC4306FC-D6C1-4BCC-AECE-48C32C1A8DAA

I thought maybe this could be of help, you can never have too many threads :) Good luck!

Artur Școlnic wrote:

Octavian, just as a side note, Java 21 finally introduced virtual or 'green' threads, the advantages over physical threads are:

• Higher scalability - Virtual threads can scale to millions, vs. thousands for platform threads.
• Lower memory usage - Virtual threads use a few KB stack vs. ~1MB for platform threads.
• Less OS overhead - Managed by the JVM, not the OS; fewer context switches.
• Simpler concurrency - Enables blocking code without blocking OS threads.
• Faster thread creation - Virtual threads are cheaper and faster to start and stop.

Here is the documentation: https://docs.oracle.com/en/java/javase/21/core/virtual-threads.html#GUID-DC4306FC-D6C1-4BCC-AECE-48C32C1A8DAA

I thought maybe this could be of help, you can never have too many threads :) Good luck!

Thanks Artur, I will take your advice into account.

I've committed some changes into 1752a/15898. I changed how target files are distributed among threads. This change was necessary because one thread was doing too much. Now, all the tasks (processing jobs) go into one shared queue, and any worker can pick a task from that list to run.

I modified all H2 data structures (H2Map, H2Set, H2MapToMap, H2MapToSet, H2ChildMap and H2ChildSet) in 1752a/15899 to be thread-safe. This way, we can run conversion with more than two threads. I continue investigating areas where threads are in the park state (suspension).

I've committed some changes in 1752a/15900 that help avoiding park states of threads. Here are some comparisons between 1752a/15900 and trunk/15920 with conversion of hotel_gui.

For these tests, 4 threads were used. The primary reason for the time difference observed is due to Code Conversion Annotations. Currently, for hotel_gui, 4 threads prove to be the most efficient configuration. This is because synchronization from AstManager continues to introduce monitor states, causing phases that were quick with 1752a to take considerably longer than with trunk.

I've noticed the same behavior during conversion of ChUI regression testing application with 1752a/15900. Code Conversion Annotations took only 10 minutes with 6 threads and with trunk it took around 19 minutes. But in the same time, with 1752a the total time was 35 minutes and with trunk the total time was around 40 minutes.

However, I happened to get random errors during conversion after other successful runs. I consider those as non-deterministic errors that I need to investigate. Additionally, I need to fix those monitors phases described above.

I've committed new changes into 1752a/15901. I safely removed profile loading for each thread and context-local implementation for RuleContainer.rules. Now, all rules and rulesets are shared without any conflicts thanks to variable scoping context-local implementation. Here are the new results of running hotel_gui conversion with 8 threads: 4:01 minutes with rules tracing on / 3:06 minutes with rules tracing off. I continue finding a solution to cover the case described in #3211-51.

I'm currently working on how to manage temp-tables processing from p2o.xml. The issue is that instead of all threads using the same class implementation, Tt1_1_1, they are also generating Tt1_1_2. The same for interface Tt1_1, generating also Tt1_2, even though tt1 is consistent across all files.

Ovidiu/Eric, do you have any suggestions of what I should be aware from p2o.xml during P2O Generation (temp-table schema files)?

Don't we have a map in which we store some reference to a temp-table that is being converted to a DMO, keyed by an instance of a class that represents the core structure of the temp-table (number, type, extent and order of fields)? Somewhere we have a process that detects that the same DMO can be used for different temp-table definitions (when the structure matches), even if they have naming differences.

I expect that when that process is used in multi-threaded mode, we must lock on that key so that any other thread that looks up the key knows to avoid creation of another instance.

Ovidiu/Eric: Please provide some QUICK feedback here to point the way.

Indeed, there are some shared maps used to check if the current temp-table has been already created using an AstKey. I added some locks in order to avoid conflicts:

=== modified file 'rules/schema/p2o.xml'
--- old/rules/schema/p2o.xml    2025-07-01 12:05:32 +0000
+++ new/rules/schema/p2o.xml    2025-07-11 12:56:37 +0000
@@ -457,6 +457,7 @@
    <variable name="denormCounter2"     type="java.lang.Integer" />
    <variable name="denormSize"         type="java.lang.Integer" />
    <variable name="likeHintsRef"       type="java.lang.ref.SoftReference" />
+   <variable name="lock"               init="createLock()"  threadSafe="true" />

    <!-- global initialization (once per pipeline) -->
    <init-rules>
@@ -771,6 +772,7 @@
          </rule>

          <!-- map tables to classes; add surrogate primary keys -->
+         <rule>lock.lock()</rule>
          <rule>evalLib('record_types') and parent.type == prog.database

             <!-- reset the field information -->
          [...]

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

               <rule>!tmpTabNames.containsKey(tmpTabKey)

                  [...]

                  <rule>!tt_view
                     <action>tmpTabNames.put(tmpTabKey, tt_impl)</action> <!-- use interface name to let views ? -->     
                  [...]

                  <rule on="false">true
                     <!-- log this access -->
                     <!-- <action>nextTTSuffixMap.containsKey(tt_interface)</action> -->
                     <action>oldPeer = tmpTabNodes.get(tmpTabKey)</action>
          [...]
@@ -1335,6 +1338,7 @@
                <action>peer.putAnnotation("like-sequential", true)</action>
             </rule>
          </rule>
+         <rule>lock.unlock()</rule>

          <!-- reset usual extent/custom extent flags for all fields-->
          <rule>evalLib('fields_all', this)

I did this for p2o_pre.xml as well. I still need to fix how jast.java is generated because a package annotation is not process properly.

We don't want a global lock. That essentially single threads the DMO conversion. Not good.

Instead, we would add a lock into the AstKey itself. Then before any generation of the associated DMO, you get the lock for the specific AstKey instance. That will fix the problem (hopefully) while remaining scalable.

Greg Shah wrote:

We don't want a global lock. That essentially single threads the DMO conversion. Not good.

Instead, we would add a lock into the AstKey itself. Then before any generation of the associated DMO, you get the lock for the specific AstKey instance. That will fix the problem (hopefully) while remaining scalable.

I fixed this in 1752a/15902. AstKey instances sharing the same hash code (indicating they refer to the same temp-table structure) now utilize a common lock.

I had a quick peek at 1752a / r15902. I understand that it is still WIP.

IMHO, we should not parallelise the ImportWorker.java. This worker is not meant to be run as the other workers during conversion. Instead, the import.xml already has dedicated means to run multiple threads in parallel and can be scaled independently.

Another thing I do not agree is the locking in TRPL. As opposed to native Java mechanism (unlocking in finally block) which guarantees the release, releasing a lock rule in TRPL requires a very solid coding to avoid the lock does not remain acquired by a problematic (x)AST, blocking this way the full multi-threaded conversion. So I wonder if we can implement some kind on locking within the TRPL language. Some kind of implicit release when the node was completely traversed. This might limit its usability but would be safer. We should at least think about it.

Ovidiu Maxiniuc wrote:

I had a quick peek at 1752a / r15902. I understand that it is still WIP.

IMHO, we should not parallelise the ImportWorker.java. This worker is not meant to be run as the other workers during conversion. Instead, the import.xml already has dedicated means to run multiple threads in parallel and can be scaled independently.

I see your point. I reverted those changes and fixed import.db targets in 1752a/15903. Now we create a single thread in PatternEngine.main, being responsible with processing of ${db.name}.p2o.

Another thing I do not agree is the locking in TRPL. As opposed to native Java mechanism (unlocking in finally block) which guarantees the release, releasing a lock rule in TRPL requires a very solid coding to avoid the lock does not remain acquired by a problematic (x)AST, blocking this way the full multi-threaded conversion. So I wonder if we can implement some kind on locking within the TRPL language. Some kind of implicit release when the node was completely traversed. This might limit its usability but would be safer. We should at least think about it.

I agree. I'll be looking into ways to implement lock release.

I think we can add this implicit release of locks in SymbolResolver.resetVariables. If that AstKey.lock was not released and the current thread is the owner, we unlock it in resetVariables. I added this change in 1752a/15904.

When temp-table DMOs are generated, I'm observing an issue where some .jast files contain asts with identical ids. I haven't identified the exact cause, but I can consistently reproduce this problem when using new shared temp-talbe.

tables.itables.i

define new shared temp-table tt1
  field  cust-num     as integer 
  field  co-num       as integer
  field  wbc          as integer
  field  timco-oper   as integer
  field  sequence     as integer
  field  tot-matl     as decimal
  field  tot-frt      as decimal
  field  tot-misc     as decimal
  field  notes        as character
  field  notes-timco  as character
  field  card-num     as character
  field  tr-date      as date
  field  tr-time      as integer
  field  tr-by        as character
  field  lab-tot-hrs  as decimal
  field  lab-est-hrs  as decimal
  FIELD  is-agg       AS LOGICAL
  FIELD  air-type     AS CHARACTER
  FIELD  air-regpart  AS CHARACTER
  FIELD  cust-name    AS CHARACTER
  FIELD  par-co-num   AS INTEGER
  FIELD  work-type    AS CHARACTER
  FIELD  detail       AS CHARACTER
  FIELD  ctype        AS CHARACTER
  FIELD  cust-by      AS CHARACTER
  FIELD  approved     AS INTEGER
  FIELD  review-time  AS INTEGER
  FIELD  review-date  AS DATE
  FIELD  run-date     AS DATE
  FIELD  run-time     AS INTEGER
  index  p-main  is unique primary cust-num co-num timco-oper sequence
  .

  define new shared temp-table tt2
  field  cust-num     as integer 
  field  co-num       as integer
  field  wbc          as integer
  field  timco-oper   as integer
  field  sequence     as integer
  field  tot-matl     as decimal
  field  tot-frt      as decimal
  field  tot-misc     as decimal
  field  notes        as character
  field  notes-timco  as character
  field  card-num     as character
  field  tr-date      as date
  field  tr-time      as integer
  field  tr-by        as character
  field  lab-tot-hrs  as decimal
  field  lab-est-hrs  as decimal
  FIELD  is-agg       AS LOGICAL
  FIELD  air-type     AS CHARACTER
  FIELD  air-regpart  AS CHARACTER
  FIELD  cust-name    AS CHARACTER
  FIELD  par-co-num   AS INTEGER
  FIELD  work-type    AS CHARACTER
  FIELD  detail       AS CHARACTER
  FIELD  ctype        AS CHARACTER
  FIELD  cust-by      AS CHARACTER
  FIELD  approved     AS INTEGER
  FIELD  review-time  AS INTEGER
  FIELD  review-date  AS DATE
  FIELD  run-date     AS DATE
  FIELD  run-time     AS INTEGER
  index  p-main  is unique primary cust-num co-num timco-oper sequence
  .

  define new shared temp-table tt3
  field  cust-num     as integer 
  field  co-num       as integer
  field  wbc          as integer
  field  timco-oper   as integer
  field  sequence     as integer
  field  tot-matl     as decimal
  field  tot-frt      as decimal
  field  tot-misc     as decimal
  field  notes        as character
  field  notes-timco  as character
  field  card-num     as character
  field  tr-date      as date
  field  tr-time      as integer
  field  tr-by        as character
  field  lab-tot-hrs  as decimal
  field  lab-est-hrs  as decimal
  FIELD  is-agg       AS LOGICAL
  FIELD  air-type     AS CHARACTER
  FIELD  air-regpart  AS CHARACTER
  FIELD  cust-name    AS CHARACTER
  FIELD  par-co-num   AS INTEGER
  FIELD  work-type    AS CHARACTER
  FIELD  detail       AS CHARACTER
  FIELD  ctype        AS CHARACTER
  FIELD  cust-by      AS CHARACTER
  FIELD  approved     AS INTEGER
  FIELD  review-time  AS INTEGER
  FIELD  review-date  AS DATE
  FIELD  run-date     AS DATE
  FIELD  run-time     AS INTEGER
  index  p-main  is unique primary cust-num co-num timco-oper sequence
  .

  define new shared temp-table tt4
  field  cust-num     as integer 
  field  co-num       as integer
  field  wbc          as integer
  field  timco-oper   as integer
  field  sequence     as integer
  field  tot-matl     as decimal
  field  tot-frt      as decimal
  field  tot-misc     as decimal
  field  notes        as character
  field  notes-timco  as character
  field  card-num     as character
  field  tr-date      as date
  field  tr-time      as integer
  field  tr-by        as character
  field  lab-tot-hrs  as decimal
  field  lab-est-hrs  as decimal
  FIELD  is-agg       AS LOGICAL
  FIELD  air-type     AS CHARACTER
  FIELD  air-regpart  AS CHARACTER
  FIELD  cust-name    AS CHARACTER
  FIELD  par-co-num   AS INTEGER
  FIELD  work-type    AS CHARACTER
  FIELD  detail       AS CHARACTER
  FIELD  ctype        AS CHARACTER
  FIELD  cust-by      AS CHARACTER
  FIELD  approved     AS INTEGER
  FIELD  review-time  AS INTEGER
  FIELD  review-date  AS DATE
  FIELD  run-date     AS DATE
  FIELD  run-time     AS INTEGER
  index  p-main  is unique primary cust-num co-num timco-oper sequence
  .

  define new shared temp-table tt5
  field  cust-num     as integer 
  field  co-num       as integer
  field  wbc          as integer
  field  timco-oper   as integer
  field  sequence     as integer
  field  tot-matl     as decimal
  field  tot-frt      as decimal
  field  tot-misc     as decimal
  field  notes        as character
  field  notes-timco  as character
  field  card-num     as character
  field  tr-date      as date
  field  tr-time      as integer
  field  tr-by        as character
  field  lab-tot-hrs  as decimal
  field  lab-est-hrs  as decimal
  FIELD  is-agg       AS LOGICAL
  FIELD  air-type     AS CHARACTER
  FIELD  air-regpart  AS CHARACTER
  FIELD  cust-name    AS CHARACTER
  FIELD  par-co-num   AS INTEGER
  FIELD  work-type    AS CHARACTER
  FIELD  detail       AS CHARACTER
  FIELD  ctype        AS CHARACTER
  FIELD  cust-by      AS CHARACTER
  FIELD  approved     AS INTEGER
  FIELD  review-time  AS INTEGER
  FIELD  review-date  AS DATE
  FIELD  run-date     AS DATE
  FIELD  run-time     AS INTEGER
  index  p-main  is unique primary cust-num co-num timco-oper sequence
  .

This file is included in multiple .p files.

• public void getTotFrt(); instead of public integer getTotFrt();
• public void setTotFrt(NumberType); instead of public void setTotFrt(NumberType totFrt);

Octavian Adrian Gavril wrote:

When temp-table DMOs are generated, I'm observing an issue where some .jast files contain asts with identical ids. I haven't identified the exact cause, but I can consistently reproduce this problem when using new shared temp-talbe.

What .jast are referring to?
In case of new shared temp-tables, if the definition is identical, a single DMO is created. It will be multiplexed at runtime. So it's normal to have references to same DMO for apparently different tables. BUT the DMO is generated only once and the ids from its .jast will be unique.

Octavian Adrian Gavril wrote:

It's definitely something wrong how TemplateWorker.graftAt works during DMOs generation. If the ids issue is not showing, then I get wrong generation of java code like:

• public void getTotFrt(); instead of public integer getTotFrt();
• public void setTotFrt(NumberType); instead of public void setTotFrt(NumberType totFrt);

The getter and setter should be emitted in TRPL by createBeanMethodDefs() function (see rules/schema/dmo_common.rules:2056). It uses simple_getter_sig and simple_setter_sig template 'code fragment' from rules/schema/java_templates.tpl. It looks the rettype annotation is missing or not taken into consideration by brewer.

In the current conversion version, I understand that each file generates a DMO for each temp-table. For files with identical temp-tables structures, the DMO from the last processed file becomes the final output, because no temp-table differences exist.

However, in multi-threaded conversion, while each thread processes a DMO for the same temp-table within its WorkArea.root, I've noticed the root instance is identical across threads at the 'saving tree' stage. Is there a TRPL mechanism that merges these results into a single AST, potentially explaining why unique ID logic is no longer provided?

Octavian Adrian Gavril wrote:

In the current conversion version, I understand that each file generates a DMO for each temp-table. For files with identical temp-tables structures, the DMO from the last processed file becomes the final output, because no temp-table differences exist.

That is true. The 'source' of the DMO interface is not know and should not matter if they are identical, as conversion identified.

However, in multi-threaded conversion, while each thread processes a DMO for the same temp-table within its WorkArea.root, I've noticed the root instance is identical across threads at the 'saving tree' stage. Is there a TRPL mechanism that merges these results into a single AST, potentially explaining why unique ID logic is no longer provided?

The (j)asts, even if they are structurally the same, come from different 4GL source files, each with its own range of ids (depending on the order of processing and location of the table definition within the source file). To my knowledge, there is no mean to identify similar trees and merge them.

• I reverted AstManager synchronization and the AstManagerPlugin is shared now.
• I fixed the unique id issue for temp-tables. The root cause was how COMPILE_UNIT IDs were generated. Even when the root instance changed, the COMPILE_UNIT ID remained the same due to the consistent file name. This way, mgr.resetNextNodeId() was called every time a .jast@ file was created. To fix this, I've implemented a condition to ensure @resetNextNodeId() is called only once per DMO file, guaranteeing unique IDs.
  

  === modified file 'src/com/goldencode/ast/AnnotatedAst.java'
  --- old/src/com/goldencode/ast/AnnotatedAst.java    2025-06-02 13:43:57 +0000
  +++ new/src/com/goldencode/ast/AnnotatedAst.java    2025-07-21 09:20:41 +0000
  
      /**
       * Get the lowercase annotation key from the {@link #ANNO_LOWER} cache.
  @@ -1045,9 +1051,30 @@
         this.clearIds();
  
         AstManager mgr = AstManager.get();
  +
         Long       id  = mgr.addTree(filename);
  -      
  -      mgr.resetNextNodeId(id);
  +
  +      if (filename.contains("dmo/_temp"))
  +      {
  +         lock.lock();
  +         try
  +         {
  +            if (!tempDMOs.contains(filename))
  +            {
  +               mgr.resetNextNodeId(id);
  +               tempDMOs.add(filename);
  +            }
  +         }
  +         finally
  +         {
  +            lock.unlock();
  +         }
  +      }
  +      else
  +      {
  +         mgr.resetNextNodeId(id);
  +      }
  +
         this.fixups(null, id, shadow);
      }
  

  Should frame generation use the same approach when dealing with shared frames?
  With the new changes, ChUI regression testing application completed its conversion in 24 minutes and 15 seconds using 16 threads, a significant improvement over the 57 minutes and 46 seconds it took with the trunk on the same machine. I need to reconvert with trunk and compare the sources to check if any differences exist.

We cannot put references to DMOs or temp-tables into generic AST support code. Please redesign the feature to expose the functionality needed and call it from TRPL rules when needed.

I redesigned the feature but I couldn't call it from TRPL because separating it from createJavaFile will let other conflicts across threads to occur. So I had to keep all functionalities together. But I moved it from AnnotatedAst to JavaPatternWorker.

I've been experimenting with converting an OO application and applied some fixes to resolve encountered errors. As a temporary measure to test the conversion process, I implemented a lock within rules/fixups/collect_procedures.rules. Moving forward, I continue implementing a more robust locking system for H2 data structures, similar to the existing AstKey mechanism.

All of these new changes are in 1752a/15906

I've committed some changes into 1752a/15907 that fix naming issue of fields/tables noticed during conversion of ChUI regression testing application. Now the schema_names_* files are generated properly. I also fixed shadow nodes creation that regressed in revision 15906.

Branch 1752a was rebased with trunk/16065, new revision is 16087. I fixed some compilation errors in 16088

• A few frame/table names are switched. For example, Tt_2 with trunk is Tt_3 with 1752a, and vice-versa. I found this difference often during conversions.
• I have two temp-tables in two separate files, defined using the like clause with the same database table. In both conversions, I have the same DMO file, but the legacy annotation is different:
  • 1752a: @Table(name = "tt", legacy = "temp-tt");
  • trunk: @Table(name = "tt", legacy = "tmp-tt").

• file1.p: def temp-table temp-tt like fwd.table1.;
• file2.p: def temp-table tmp-tt like fwd.table1.
  where table1 is a database table.

I converted a large GUI application and I have a compilation error due to procedure names conflicts:
In the same file, I have procA and proc_A. The resulting code is not compiling because it has two procedures defined with the same name:

   @LegacySignature(type = Type.PROCEDURE, name = "proc_A")
   public void procA()
   {
      internalProcedure(Start.this, "proc_A", new Block((Body) () -> 
      {
         message("Test");
      }));
   }

   @LegacySignature(type = Type.PROCEDURE, name = "procA")
   public void procA()
   {
      internalProcedure(Start.this, "procA", new Block((Body) () -> 
      {
         message("Test1");
      }));
   }

When converting a single file, _n (where n is an integer) is appended to the filename to prevent naming conflicts.

I think that the root cause of the issue described in #3211-80 is a global variable declared as threadSafe but it shouldn't be shared. That variable was marked as threadSafe because of its initialization in <init-rules>. So, I had to fix the issue discussed in #3211-51, regarding the initialization of non-shared variable using <init-rules>. I implemented a possible solution and now I'm running conversion of a few projects and if there are no regressions I will commit the fix.

I've committed new changes in 1752a/16090 with a solution for #3211-51. Now, each setValue call of non thread safe variables is applied to each thread when run within an init-rules block.

• Added specific locks for FrameAstKey for synchronized processing of the same FrameAstKey. It's the same approach as for AstKey processing during DMO generation;
• All variables that are H2 data structures are now marked as threadSafe because these are actually sharing the same table in H2 database.
• Refactored some .rules files that used blocks like:

  <rule>not myVar.containsKey(key)
     [...]
     <action>myVar.put(key, value)</action>
  </rule>
  

  , where myVar is an H2 data structure variable. Now these blocks are turned into atomic operations, using putIfAbsent method, implemented in each class.
• Removed the quick fix mentioned in #3211-76 from rules/fixups/collect_procedures.rules.

I tested conversion for the other projects and no new errors occur. The issue described in #3211-80 is fixed now. I am currently having problems with populating java files with the proper content. For an OO application, I have this type of output for two classes:

package [...];

import com.goldencode.p2j.util.*;

import static com.goldencode.p2j.util.BlockManager.*;

public class ClassName
extends ${extends}
implements null,
           null
{
   public void __ClassName_execute__}

        <action>clsName = superCls</action>
         <while>clsName != null
            <action>clsVars = classVars.get(clsName)</action>

            <rule>clsVars != null
               <action>iter = clsVars.iterator()</action>
               <action>aref = null</action>
               <while>iter.hasNext()
                  <action>varid = #(long) iter.next()</action>
                  <action>aref = getAst(varid)</action>

                  <action>execLib("process_variable", varid)</action>
               </while>

               <!-- persist the AST for the super-class -->
               <rule>aref != null
                  <while>aref.parent != null
                     <action>aref = aref.parent</action>
                  </while>

                  <action>persist(aref, sprintf("%s.ast", aref.filename), true, false)</action>
               </rule>
            </rule>

            <action>clsName = cls2super.get(clsName)</action>
         </while>

• One issue is that some threads could use the same iterator in order to go through all variables of the super-class.
• Another one is that getAst can be called before the final version of super-class AST is persisted successfully.

Does current conversion prioritize the super-class persistence over the current processed class? How this functionality is guaranteed?

Octavian Adrian Gavril wrote:

Does current conversion prioritize the super-class persistence over the current processed class? How this functionality is guaranteed?

We recursively parse the superclasses, interfaces and every object graph dependency such that they are fully parsed and available before the referencing 4GL code continues parsing. In single threaded mode that is enough.

For multithreading, we must manage that more explicitly. After parsing, we will have other moments in which we have to wait for some dependent class to be converted (e.g. to get the converted names to reference) so even the later steps will have to be organized and synchronized well. Converting the object graph in the right order will help make the conversion more efficient.

I also wonder if we need to be smarter about thread usage such that if we would block waiting for given resource, that we let that same thread continue with another job and come back to this job when the block is cleared.

For now, I added a locking system over processing of the same class. This way, no conflicts occur when the super-class is processed and its AST is modified by inheriting classes in the same time. I added this temporary fix into 1752a/16092.

I tested with an OO application and now the conversion finish successfully. I have a few compiling errors because of some temp table DMO classes that have switched methods.

Current issue can be reproduce with the following testcase:
file1.p:

DEFINE TEMP-TABLE tt NO-UNDO
  FIELD num AS INTEGER
  FIELD n AS CHAR
  INDEX num IS PRIMARY num
  .

DEFINE TEMP-TABLE tt NO-UNDO
  FIELD num AS INTEGER
  FIELD name AS CHAR
  FIELD initial AS CHAR 
  INDEX num IS PRIMARY num
  .

The resulted DMOs are Tt_1.java, Tt_1_1.java and Tt_1_2.java. The output with trunk is Tt_1.java, Tt_1_1.java, Tt_2.java and Tt_2_1.java.

I think that instead of creating new interfaces like Tt_2.java and Tt_2_1.java, Tt_1.java is overwritten with the content of Tt_2.java and Tt_1_2.java is identical to Tt_2_1.java.

I've committed new changes in 1752a/16093 that fix locking for temp-table DMOs generation. This way, the testcase from #3211-90 is fixed, hotel_gui, ChUI regression testing application and an OO application are converted successfully using 1752a. No compilation errors. I continue testing a large GUI application conversion and the runtime environment of the other applications. The differences are related to the names of the temp-tables DMOs. Either they are different names (different indexes), or they are changed to the names of other tables.

• I see that a few profiles are loaded at the beginning and then are stored in ConversionPool.pool. This way, applyGlobal() calls use AstSymbolResolver.ruleScope as null because the actual set of PatternEngine.this as rule scope was made at initialization time. So I had to reset the rule scope before calling applyGlobal in PatternEngine.run().
• I've also discovered that some logic from complex constructors is not executed due to context-local implementation of workers classes. I add a few context.get() to trigger that logic where needed.

• I've also removed the context-local from src/com/goldencode/p2j/cfg/Configuration.java as it generated a few inconsistencies about some flags;
• Moved the unlock() method call to avoid NPE.

Now both servers of large GUI application and OO application, run successfully. I did some smoke testing and everything seems fine. I started to run performance/unit tests but I think a rebase is needed due to some memory leak regression fixed in trunk but not in 1752a.

Rebased branch 1752a with trunk/16107, new revision is 16136.

Fixed some compilation errors after rebase in revision 16137.

I run performance tests for an OO application and all tests passed. I run unit tests for a large GUI application, as well. The results are the same as those after running with trunk/16107.

Fixed some differences from cvt/data in 1752a/16138. This way, files from dmo/_meta are now identical.

I removed locks mentioned in #3211-88 and implemented the idea from #3211-87. If I want to process class B.cls which inherits A.cls, but A.cls is not marked as processed, then I move the processing of B.cls at the end of the queue.

I tested these changes with an OO application but at Business Logic Base Structure phase I was getting this kind of errors:

Passing unknown value literal to builtin func! [dump tree that generated the error]

The .ast files are generated properly, without any missing annotations. But the .jast files are broken.

I modified the code to be applied to Code Conversion Annotations phase only, as this was the problematic processing. I'm retesting the conversion now.

The conversion completed successfully. I committed the new changes into 1752a/16139.

I went through all modified files, did some clean up and added history entries / javadocs where it was missing. These changes are committed into 1752a/16140.

• there are different numbers for frame setters:

   public interface FrameFBhdr_1
   extends CommonFrame
   {
  -   public void setExpr17(character parm);
  +   public void setExpr20(character parm);
  
  -   public void setExpr17(String parm);
  +   public void setExpr20(String parm);
  
  -   public void setExpr17(BaseDataType parm);
  +   public void setExpr20(BaseDataType parm);
  
  -   public LiteralWidget widgetExpr17();
  +   public LiteralWidget widgetExpr20();
  
  -   public void setExpr18(character parm);
  +   public void setExpr21(character parm);
  
  -   public void setExpr18(String parm);
  +   public void setExpr21(String parm);
  
  -   public void setExpr18(BaseDataType parm);
  +   public void setExpr21(BaseDataType parm);
  
  -   public LiteralWidget widgetExpr18();
  +   public LiteralWidget widgetExpr21();
  
  -   public void setExpr19(character parm);
  +   public void setExpr22(character parm);
  
  -   public void setExpr19(String parm);
  +   public void setExpr22(String parm);
  
  -   public void setExpr19(BaseDataType parm);
  +   public void setExpr22(BaseDataType parm);
  
  -   public LiteralWidget widgetExpr19();
  +   public LiteralWidget widgetExpr22();
  
   }
  

• A missed import statement in some files from an OO aplication:

   import static com.goldencode.p2j.util.BlockManager.*;
   import static com.goldencode.p2j.util.InternalEntry.Type;
   import static com.goldencode.p2j.util.logical.*;
  +import static com.goldencode.p2j.util.ProcedureManager.*;
   import static com.goldencode.p2j.util.TextOps.*;
  
   /**
  

  This doesn't generate any compile error, but for some reason, the import AST is not attached in the .jast file.

I've analyzed the conversion logs with 1752a vs trunk and I can see that DMOs generation phases are improved with 1752a and running them with one thread only is not a good idea (Generate Data Model Objects (temp-table DMOs) and Generate Java Source Data Model Objects (DMOs)). But the non-deterministic naming is executed in Pre-processing P2O Generation and P2O Generation. These phases take longer time compared to single threaded conversion. This happens due to locking implementation from p2o_pre.xml/p2o.xml and synchronization from NameConverterWorker. Here are the elapsed times for each phases obtained from conversion of a large GUI application:

I wrote in bold the minimum times obtained. I think we can safely run those p2o phases in single threaded mode. IMHO, I think we should do this for all phases that are faster with a single thread. This way, we get the fastest conversion time for each phase and in the same time, we avoid non-deterministic naming of DMOs.

I don't want to artificially limit our future improvements by forcing single threading for any significant portion of the conversion. The idea to use a pre-calc of names in a single threaded mode is a temporary workaround for the non-deterministic DMO name problem. Even that will be eliminated at some point.

This is happens due to locking implementation from p2o_pre.xml/p2o.xml and synchronization from NameConverterWorker.

Please dig into this. Provide more detail about these implementations and we will discuss how to make them more efficient.

All NameConverterWorker state is context-local implemented for runtime conversion. I had to share that state between threads in order to generate names reports properly (schema_names_*.rpt). So, I added synchronization for that state where was needed. These changes are mentioned in #3211-77. Now, I'm testing if this can be safely remove.

All NameConverterWorker state is context-local implemented for runtime conversion. I had to share that state between threads in order to generate names reports properly (schema_names_*.rpt). So, I added synchronization for that state where was needed. These changes are mentioned in #3211-77. Now, I'm testing if this can be safely remove.

The part I don't understand here is why use context local data if it needs to be shared?

Greg Shah wrote:

All NameConverterWorker state is context-local implemented for runtime conversion. I had to share that state between threads in order to generate names reports properly (schema_names_*.rpt). So, I added synchronization for that state where was needed. These changes are mentioned in #3211-77. Now, I'm testing if this can be safely remove.

The part I don't understand here is why use context local data if it needs to be shared?

That was already implemented with context-local, before this task. That implementation was added for runtime conversion. I didn't remove that in order to keep runtime conversion functional.

I think I found another fix for naming problem of fields/tables. I removed synchronization and shared NameConverterWorker.WorkArea.defaultConverter only, not the all worker's state. I'm testing the changes to check if elapsed time is improved.

Added that fix in 1752a/16141. I added single threaded mode as well, but it's not used. I tested on a few apps and elapsed times related to p2o are improved. I'm testing the conversion with a large GUI application.

The large GUI application converted successfully but I have a few class properties that are called varName instead of varNameVar1 (this is how they're converted with trunk). I'm investigating this converting issue.

I could reproduce the issue described in #3211-10 using the following testcase:
B.cls:

class B: 

define protected variable D as decimal no-undo.

define protected variable D1 as decimal no-undo.

define protected variable D2 as decimal no-undo.

    constructor public B (  ):
        D = 0.5.  
    end constructor.

    destructor public B ( ):
        // test
    end destructor.

end class.

class A inherits B:

    define  temp-table D no-undo  
    field txt                              as character.

    constructor public A():
        D = 0.5.
        // test
    end constructor.

    destructor public A():
        // tests
    end destructor.

end class.

class A1 inherits B:

    define  temp-table D1 no-undo  
    field txt                              as character.

    constructor public A1():
        D1 = 0.5.
        // test
    end constructor.

    destructor public A1():
        // test
    end destructor.

end class.

class A2 inherits B:

    define  temp-table D2 no-undo  
    field txt                              as character.

    constructor public A2():
        D2 = 0.5. 
        // test
    end constructor.

    destructor public A2():
        // tests
    end destructor.

end class.

I think that each inheriting class modify and save the inherited class with its own file version. The changes from revision 16139, that handle OO files processing order, fix only the case of inheriting class vs inherited class. This issue is across classes which inherit the same super-class.

I fixed the issue described above in 1752a/16142. I'm retesting the large GUI application conversion.

As we discussed in today's meeting, there is an unused import that is attached with trunk and it's not attached with 1752a. The responsible file is convert/oo_calls.rules:

  <ascent-rules>
      <rule>this.type == prog.object_invocation and 
            (downPath(this, prog.object_invocation, prog.class_event) or 
             (this.firstChild.type == prog.class_event and parent.type != prog.object_invocation))
         [...]
         <rule>(ref2.type == prog.kw_subscrib or ref2.type == prog.kw_unsubscr)
         [...]
            <rule>ref2.nextSibling.type == prog.string
               <action>
                  createJavaAst(java.static_method_call, "thisProcedure", closestPeerId, idx)
               </action>
               <action>pmimport = true</action>
            </rule>
            [...]
            <post-rules>
               <rule>pmimport
                  <action>help.createStaticImport("com.goldencode.p2j.util.ProcedureManager.*")</action>
               </rule>
            [...]

The issue related to files with the same names can be fixed as DMOs naming, by calculate the name in advance on singlethreaded mode. Now, start.w and start.p are named Start.java and Start_1.java. This is non-deterministic for multithreaded conversion.

The last issue is Generate WSDL files phase. These files have mixed content from conversion to conversion. Maybe we can order each file content.

oo_calls.rules does not set pmImport = false in the init-rules section - so this var is never reset for the AST, and is inherited from the previous run.

Constantin Asofiei wrote:

oo_calls.rules does not set pmImport = false in the init-rules section - so this var is never reset for the AST, and is inherited from the previous run.

Oh, so with multithreaded conversion, pmImport is initialized to false for each thread and then set to true and will remain with that value for that thread. This means that there are unused import statements in multithreaded conversion as well, but less than singlethreaded conversion.

I've committed new changes in 1752a/16143.

I moved the class name suffix calculation from p2o.xml to p2o_pre.xml. I found p2o_pre.xml the perfect place where that processing can be executed, as it was processing the first layer of temp-table interface names until now. So, p2o_pre.xml is executed on a single thread.
Then, the generated names are extracted in p2o.xml. This way the deterministic generation of suffixes is guaranteed.

I tested with a few testcases and with hotel_gui and now there are no more differences between DMOs. I continue testing with the other apps.

I've committed some changes into 1752a/16144. These include fixes for deterministic generation of sql names and class names. This way, the issue related to same file names is fixed. I continue investigating the problem with non-deterministic artifacts content.

Octavian Adrian Gavril wrote:

I've committed some changes into 1752a/16144. These include fixes for deterministic generation of sql names and class names. This way, the issue related to same file names is fixed. I continue investigating the problem with non-deterministic artifacts content.

Some additional notes about these changes. I had to move some code from naming.rules into a separate conversion phase. This was needed in order to run that generation in a single thread. The phase is called Compute class names and is added in ConversionDriver.back(), between Unreachable Code Analysis and Code Conversion Annotations Prep. I couldn't add that code in annotations_prep.xml as a new rule-set because that would have involved running of annotations_prep.xml in a single thread.

• I analyzed frame_generator_pre.xml more carefully and I think that is responsible with interface name generation only. So I considered useful to run it in a single thread. No other new phase for frame interface names generation is needed.
  This way, the names calculation is deterministic for frames as well and it's seems that #3211-102 is fixed too.
• I changed the maps from both NameMappingWorker and ServiceSupport into TreeMap to guarantee that the file content will be the same from conversion to conversion.
• I tested the last revision with many apps and I checked the new conversion phase added in 1752a/16143. It seems that Compute class names from 16143 took too much time. I made it faster in 16144 by processing only the files that have duplicates. (e.g. a/b/start.w and a/b/start.p).

The current issue that I'm working on is related to custom properties added to temp-tables DMOs by files which use the same interface. By custom properties, I'm referring to stuff like no-undo, index names and so on. With trunk, the last processed file writes the properties of its own interface version. I'm gonna reproduce this behavior by delegate a single file for this processing.

The current issue that I'm working on is related to custom properties added to temp-tables DMOs by files which use the same interface. By custom properties, I'm referring to stuff like no-undo, index names and so on. With trunk, the last processed file writes the properties of its own interface version. I'm gonna reproduce this behavior by delegate a single file for this processing.

Actually, this is a bug in trunk. I'm not sure we want to duplicate the bug, but I also don't know what the solution would be.

Alex/Ovidiu/Eric?

Alex/Ovidiu/Eric?

My POV is that we should converge to what trunk has. We can agree that trunk is broken, but at least is broken in a way that customers already tested and confirmed is OK . If we generate other DMOs, then latent issues may surface.

OK

Actually, we kind of rely on this behaviour to make sure the DMO interface is created and the fact it is overwritten guarantees it will not be created multiple times causing other errors, like compilation errors. Of course, we can implement a 'voting' machine to pick a single source to generate the DMO, but it is not yet done.

This is not correct and is pretty non-deterministic. We should do it right, I think, if not now (this task will inevitable cause lots of other differences in converted code I guess), then sometime in the future.

I finished an initial solution for the issue and started testing with a few apps. I found some problems during ChUI regression testing app conversion and some temp-table implementations are not properly generated. I continue investigating this.

I've committed new changes into 1752a/16146.

I added implementation that establish which source file will generate which DMO in a single threaded execution. This assignation is executed in p2o_pre.xml. The check before persisting the DMO is in java_dmo.xml. The solution works fine. I tested the ChUI regression testing app and an OO application and the DMOs are identical to what trunk does.

During testing the OO app I discovered an edge case for the third point of #3211-121. I added changes to fix naming of files like start.p, sta-rt.p and sta_rt.p. All of those will end up by converting as staRt.p (+ staRt_1.p and staRt_2.p). That's the reason why I had to use the converted name instead of original filenames.

I started the testing with the latest changes. ChUI regresion testing app converted successfully so far. I'll be back with the results of other apps.

I analyzed the results with other apps and it seems that Detecting Frame Interfaces takes too long to execute in a single thread with 1752a. I have to rework the switching between multithreaded and singlethreaded.

About the content of the frames, I tested these changes to get a deterministic content with frame operations:

=== modified file 'rules/convert/frame_generator.xml'
--- old/rules/convert/frame_generator.xml    2025-08-19 09:07:02 +0000
+++ new/rules/convert/frame_generator.xml    2025-09-15 14:06:01 +0000
@@ -1307,7 +1307,7 @@
             <action>isText = frame.get(frameText).equals("TextWidget")</action>
             <action>isStreamIO = frame.get(frameStreamIO) != null</action>

-            <action>map = #(java.util.Map) create('java.util.LinkedHashMap')</action>
+            <action>map = #(java.util.Map) create('java.util.TreeMap')</action>
             <action>wlist = #(java.util.List) frame.get(frameWid)</action>
             <action>iter = wlist.iterator()</action>

@@ -1440,10 +1440,10 @@

          <rule>true
             <action>map = #(java.util.Map) 
-                              create('java.util.LinkedHashMap')</action>
+                              create('java.util.TreeMap')</action>

             <action>arrays = #(java.util.Map) 
-                              create('java.util.LinkedHashMap')</action>
+                              create('java.util.TreeMap')</action>

             <action>wlist = #(java.util.List) frame.get(frameWid)</action>

I'm not sure how this can be helpful in order to always get the same frame operations order because each file is processed by a single thread. Based on my understanding, this happens with trunk as well. Does anyone know a better place for this kind of changes?

Octavian Adrian Gavril wrote:

I analyzed the results with other apps and it seems that Detecting Frame Interfaces takes too long to execute in a single thread with 1752a. I have to rework the switching between multithreaded and singlethreaded.

I think the cause of this behavior is that unused threads keep running during singlethreaded execution. I added new changes in 1752a/16147 that stop threads execution during singlethreaded phases. I tested with a few apps and it seems to work well. I continue testing.

Shouldn't each PatternEngine invocation create a new thread pool? Making a single-threaded process would be as simple as ensuring that the number of threads is 1 for specific PatternEngine invocations.

Greg Shah wrote:

Shouldn't each PatternEngine invocation create a new thread pool? Making a single-threaded process would be as simple as ensuring that the number of threads is 1 for specific PatternEngine invocations.

Originally, the thread pool implementation was static. But now it seems clearer to me to make it instance-specific. I will refactor that area.

I analyzed the results of large GUI app conversion and I noticed that we are losing the static context everytime we create new threads. I think that was a good reason for static thread pool implementation.

For example, NameConverter has static fields like reservedSQL which stores the reserved keywords that SQL uses. This is populated only in a few phases and with trunk there are other phases that use those based on its previous population. Files like order.p will be converted as Order_.java because reservedSQL has the order element stored by previous phases.

• We can store all of the static context and reassign them to the new threads. But this seems a heavy process.
• We keep the same threads and 'pause' the execution of threads and let only one thread responsible for singlethreaded phases. Or we can test how locking/synchronization works on the main tasks queue.

Octavian Adrian Gavril wrote:

• We keep the same threads and 'pause' the execution of threads and let only one thread responsible for singlethreaded phases. Or we can test how locking/synchronization works on the main tasks queue.

One disadvantage here is that we cannot guarantee that the thread that adds the values ​​to reveredSQL will be the same thread that uses them when converting the class name. Do you think this is the only situation where different conversion phases depend on each other through static fields? If so, this loss of static context is not a real problem and we can fix this case only (order.p -> Order_.java)

I think we should let the current thread pool as a static member because there are several static actions of JavaPatternWorker that need to be performed by threads before/after the usage of a PatternEngine instance (e.g. those from generation of DMOs, POJOs, frames and menus).

Ovidiu, could you please clarify if the check using reservedSQL is required?
NameConverter.resolvePossibleKeywordConflict:

         case TYPE_CLASS:
            hit = reservedJava.contains(lcPossible) ||
                  reservedFWD.contains(possible)    ||
                  reservedSQL.contains(lcPossible);

If resetDialects() method wouldn't be called in p2o.xml, reservedSQL would be empty.

What do you mean by "static context"? Anything that is a static member will exist for the lifetime of the JVM. Conversion phases all run in the same JVM process so any static data should always exist.

It is valid to have static data that is used across phases. That is expected and is a good thing. I don't understand what the problem is here.

Any such static data needs to be properly thread-safe so that all access is syncronized or handled via some concurrency mechanism. Otherwise, I don't see a problem.

Greg Shah wrote:

Any such static data needs to be properly thread-safe so that all access is syncronized or handled via some concurrency mechanism. Otherwise, I don't see a problem.

I moved all of the static state in a context-local implementation. This way, some values are visible for a specific thread.

Any such static data needs to be properly thread-safe so that all access is syncronized or handled via some concurrency mechanism. Otherwise, I don't see a problem.

I moved all of the static state in a context-local implementation. This way, some values are visible for a specific thread.

Anything that needs to be shared across threads or across phases must be static and made safe for concurrent access. Using ContextLocal for this is not right.

reservedSQL is cleared and repopulated everytime a new DATABASE token is found, using resetDialects(). This seemed to be specific for each AST that is processed so I considered it good to be moved in a context-local implementation.

Aren't reserved keywords things that we know in advance and they don't change? Why would they need to be dynamically populated?

That check using reservedSQL can't be removed because TYPE_CLASS is used for converting packages and DMO names as well.

         case TYPE_CLASS:
            hit = reservedJava.contains(lcPossible) ||
                  reservedFWD.contains(possible)    ||
                  reservedSQL.contains(lcPossible);

This way, currentSchemaName will be the only context-local value from NameConverter and based on that we decide which dialects should be used for checking keywords conflicts.

I think a solution would be to move reservedSQL as static field (no context-local) and to refactor it as a map that contains all dialects and schemas. What do you think?

Yes, this is exactly what I am suggesting.

However, we will have no currentSchemaName in Code Conversion Annotations Prep, where naming of files is executing and uses reservedSQL. We should check all the dialects in this case?

Why does the schema name matter here? A reserved SQL keyword is reserved for a given dialect no matter what schema is being processed.

Greg Shah wrote:

Why does the schema name matter here? A reserved SQL keyword is reserved for a given dialect no matter what schema is being processed.

I asked that because in NameConverter.resetDialects() dialects are loaded based on a given schema.

Octavian Adrian Gavril wrote:

Greg Shah wrote:

Why does the schema name matter here? A reserved SQL keyword is reserved for a given dialect no matter what schema is being processed.

I asked that because in NameConverter.resetDialects() dialects are loaded based on a given schema.

I think currentSchemaName should not exist. We should not care about the schema here at all.

Reserved keywords are known in advance, they are "global" (for ANSI keywords) or they are specific to a dialect, but either way they do not vary. Each dialect should be able to return its list of reserved keywords.

Instead of an overly simplified map lookup that is constantly being swapped out by schema, we need a helper method to check against the global list and against the dialect-specific list.

I the case of TYPE_CLASS, I think we should be checking all dialects. In the case of TYPE_TABLE, TYPE_COLUMN or TYPE_INDEX we should know which dialect we are dealing with and process only the global keywords and those that are dialect-specific to that dialect.

Eric/Ovidiu: Do you see anything wrong with my logic here? The result will be at least as good as what we do now and it will be less complicated from the perspective that we will eliminate the currentSchemaName and we won't have to constantly process resetDialects(). We can iterate though the dialects at startup and store a mapping of the dialect names to their keyword maps. Then everything is static and pretty straightforward.

I've committed new changes in 1752a/16148 that include some clean up code and cosmetic changes.

I had a short discussion with Octavian earlier, today. He was wondering why we need to check if a name conflicts with SQL reserved keywords and if making reservedSQL is a good idea. Here is my answer:

The need for checking the SQL keyword is necessary because we have the intermediary language FQL (stands for FWD Query Language), very similar to HQL. The conversion emits these queries in generated Java code, but at runtime we need to parse the FQL (it is a dialect independent language) and adjust it so that dialect-specific quirks are correctly handled. If a class name collide with a keyword will make the FQL invalid/not parsable.

Related to reservedSQL. This set holds the reserved keywords for the current schema only. Each schema/database can be configured in cfg/p2j.cfg.xml to support a different set of dialects. To allow the keywords from a dialect which was used in a previous schema, but not configured for the current one, the set is reset and strictly only the keywords which could potentially cause collisions are stored. Bottom line: each database with its own reserved keyword set.

I do not know how this parallelisation is implemented in 1752a branch. The simplest approach would be to have one thread for each schema. Indeed, this will not distribute the load equally (there might be a single database, or their sizes very different). The advantage is that it is simpler to implement, by making the to be reservedSQL thread local.

• require to scan all schemas and collect all dialects;
• alter some table/column names, even if the keywords of the dialects they will be used will not collide with them.

I prefer to eliminate the schema-specific nature of the design. I don't see the value it provides in this case. Please see my suggested approach in #3211-146.

The 'reset' happens only once for each namespace defined in p2j.cfg.xml, to avoid 'blocking' more keywords than necessary. It was implemented as result of #2332-43. The usage of currentSchemaName is just a safeguard and the marker should actually never be matched unless the same namespace is defined twice, but this is wrong.

If we just pass enough information to the name converter to know the dialect, then none of this schema knowledge or state is needed.

Ovidiu Maxiniuc wrote:

The 'reset' happens only once for each namespace defined in p2j.cfg.xml, to avoid 'blocking' more keywords than necessary. It was implemented as result of #2332-43. The usage of currentSchemaName is just a safeguard and the marker should actually never be matched unless the same namespace is defined twice, but this is wrong.

So, the only conversion phase where the reversedSQL is cleared and repopulated multiple times is P2O Generation (database schema files)? Then, in P2O Generation (temp-table schema files) reservedSQL will be populated for _temp only. So later, in Code Conversion Annotations Prep, the elements used will be those related to _temp.

• We can execute P2O Generation (database schema files) in a singlethreaded mode and make reserverdSQL static field (no context-local).
• We keep reservedSQL as context-local but in naming phase (Code Conversion Annotations Prep) we 'reset' dialects for _temp.

Both of these approaches keep the current trunk behavior, allowing us to address the reserved SQL keyword conflicts later in a separate task.

I don't want to single thread things that are not needed. I prefer not to leave this dependency on state modification which is not needed.

Please implement my approach from #3211-146. It will solve the issues and will do so in a way that is permanent rather than a workaround that will need fixing later.

Greg Shah wrote:

I don't want to single thread things that are not needed. I prefer not to leave this dependency on state modification which is not needed.

Please implement my approach from #3211-146. It will solve the issues and will do so in a way that is permanent rather than a workaround that will need fixing later.

Sure! I'm on it.

• refactoring of how SQL keyword conflicts are solved following instructions mentioned in #3211-146. I tested the solution with a few testcases and it seems fine. The original problem, mentioned in #3211-132, is solved as well (order.p example).
• removing sharing of the defaultConverter from NameConversionWorker. I had an issue with MetaUser table that had differences related to filed names due to some flag that shouldn't be shared. In the last revision, the entire NameConverter instance was shared in NameConverterWorker to generate names for fields/tables that use cfg/datanames.txt. We discussed about this implementation in #3211-105 and sharing the converter seemed the single solution at that time. I removed that sharing and added a constructor call for each thread:
  

    public NameConverterWorker()
     {
        super();
  
        setLibrary(new Library());
  
        // re-initialize the default context-local NameConverter, as the defaults may have changed.  this is
        // required for static conversion, as the datanames.xml is loaded when processing schema DMOs.  for
        // dynamic conversion, datanames.xml has no benefit, and is OK to not be used.
        context.get().defaultConverter = new NameConverter();
        PatternEngine.addThreadsJob(() -> context.get().defaultConverter = new NameConverter());
     }

  
  Based on my understanding, this constructor call trigger loading of all values from cfg/datanames.txt. So, that could explain why without that call, the names are not mapped due to cfg/datanames.txt.
• Replaced HashMap with LinkedHashMap in frame_generator.xml. I have to test these changes with a few consequential conversions.

I continue testing the new changes with a few apps.

I fixed the wrong dialect name used in the last revision. I tested the changes from frame_generator.xml and these worsened the consistency of the order of frame operations. So, I removed them. The new revision is 1752a/16150.

I've identified the root cause of the non-deterministic frame operation order. The issue originates from the parsing phase, specifically with the generation of the KW_ENABLE AST. The core problem is the use of HashSet throughout progress.g and in the SymbolResolver.getFrameFields method. By replacing all HashSet instances with LinkedHashSet, I was able to fix my test case. I'm now testing this solution with other applications, and if the results are positive, we can move this task to review.

Rebased 1752a to latest trunk/16184, the branch is now at revision 16229.
Greg, Constantin, please review.

I started testing incremental conversion and committed new fixes in revision 1752a/16230.

• set incremental conversion to single thread only execution;
• fixed singlethreaded conversion;
• so far I have used tmpTabNames in both p2o_pre.xml and p2o.xml. But this triggered some errors for incremental conversion because I used clear() between usages. In order to fix that, now I use a dedicated map in p2o_pre.xml.

set incremental conversion to single thread only execution;

What do you mean by this? I prefer not to limit incremental mode to a single thread. Is there a requirement for that?

I thought that it will be useful to use a single thread for incremental conversion as the number of files would be always small. But we can keep singlethreaded mode as default and enable the customization for number of threads, just like the full conversion.

Please do.

Greg Shah wrote:

Please do.

Done in rev 16232. ✅

I was analyzing the impacts to the runtime conversion code and recalled your earlier note (#3211-11) about run() and run(String). I agree that we should implement multithreaded execution for both methods. I'm going to start on that work now. Do you have any test suggestions for validating this multithreaded change?

Do you have any test suggestions for validating this multithreaded change?

The runtime usage (run()) is only used for 2 things:

• Creating new DMOs (CREATE TEMP-TABLE statement).
• Creating dynamic queries (QUERY-PREPARE() usage).

I would focus on testcases that have these features AND which are already done in multiple sessions at runtime. For example, 5 users simultaneously doing a combination of new DMOs and dynamic queries.

I analyzed the current implementation of runtime conversion. From my understanding, now we attribute a stack with PatternEngine instances to each conversion phase that we have. The population of these stacks is done in ConversionPool.initialize() but a single instance is added:

     // add pattern engines
      for (ConversionProfile profile : ConversionProfile.values())
      {
         // TODO: use directory-based configuration to initialize number of engines per profile
         instance.addEngine(profile);
      }

  private PatternEngine popEngine(ConversionProfile profile)
   {
      Stack<PatternEngine> stack;
      synchronized (pool)
      {
         stack = pool.get(profile);
      }

      PatternEngine engine;
      synchronized (stack)
      {
         while (stack.isEmpty())
         {
            try
            {
               stack.wait();
            }
            catch (InterruptedException exc)
            {
               // TODO: check whether spurious or real termination
            }
         }

         engine = stack.pop();
      }

      return engine;
   }

IMHO, we cannot implement any multithreaded execution the way it is implemented for full conversion. To achieve concurrency, we must either complete the TODO to deploy multiple PatternEngine instances per profile, OR we can take advantage of the engine-sharing capability mentioned in 1752a, provided the threads that split the jobs are the actual Conversation threads that initiate the conversion. We should move storeObjects to Context-local implementation in order to avoid conflicts across returned results by ConversionPool.runTask()

Octavian Adrian Gavril wrote:

IMHO, we cannot implement any multithreaded execution the way it is implemented for full conversion. To achieve concurrency, we must either complete the TODO to deploy multiple PatternEngine instances per profile, OR we can take advantage of the engine-sharing capability mentioned in 1752a, provided the threads that split the jobs are the actual Conversation threads that initiate the conversion. We should move storeObjects to Context-local implementation in order to avoid conflicts across returned results by ConversionPool.runTask()

Do you have any thoughts about second idea? My testcase runs the same query multiple times from different clients, and I've disabled caches to force re-parsing every time. Do you think the concurrent processing of the identical AST by several clients using the same engine would be problematic?

start.pstart.p

DEFINE VARIABLE iTestRuns AS INTEGER INITIAL 500 NO-UNDO.
DEFINE VARIABLE iRunTimeSeed AS INTEGER NO-UNDO.
DEFINE VARIABLE cLogFile AS CHARACTER INITIAL "TestLog.txt" NO-UNDO. /* <--- Log File Name */
DEFINE VARIABLE cLogTime AS CHARACTER NO-UNDO.

iRunTimeSeed = INTEGER(ETIME / 1000) MOD 10000.
cLogTime = STRING(TODAY) + " " + STRING(TIME).

def temp-table tt1 field f1 as int
                         field f2 as char.

PROCEDURE RunDynamicStressTest:
    define variable iloop as integer no-undo.
    DEFINE VARIABLE cTestID AS CHARACTER NO-UNDO.
    def var h as handle.

    cTestID = "SESSION-" + STRING(iRunTimeSeed).

    OUTPUT TO VALUE(cLogFile) APPEND.
    PUT UNFORMATTED "--- Stress Test Started: " cLogTime " (" cTestID ") ---" SKIP.
    OUTPUT CLOSE.

    DO iLoop = 1 TO iTestRuns:
         OUTPUT TO VALUE(cLogFile) APPEND.
        PUT UNFORMATTED cTestID " Loop " iLoop ": Preparing Query..." SKIP.
        OUTPUT CLOSE.

        create tt1.
        tt1.f1 = 4.
        release tt1.

        def query hquery for tt1.

        do transaction on error undo, throw:
            def var ch1 as char.
            ch1 = "FOR EACH tt1 WHERE tt1.f1 < 9 and tt1.f2 = 'bc'".

            query hQuery:QUERY-PREPARE(ch1).
        end.    

        OUTPUT TO VALUE(cLogFile) APPEND.
        PUT UNFORMATTED cTestID " Loop " iLoop ": Completed and resources released." SKIP.
        OUTPUT CLOSE.

    END. /* DO iLoop */

    OUTPUT TO VALUE(cLogFile) APPEND.
    PUT UNFORMATTED "--- Stress Test Finished ---" SKIP.
    OUTPUT CLOSE.

END PROCEDURE.

RUN RunDynamicStressTest.

we can take advantage of the engine-sharing capability mentioned in 1752a

Can you be more specific here? The current approach for runtime queries already does share pattern engine instances. I don't see anything in #1752 that describes an approach to use here.

provided the threads that split the jobs are the actual Conversation threads that initiate the conversion. We should move storeObjects to Context-local implementation in order to avoid conflicts across returned results by ConversionPool.runTask()

In a non-runtime conversion using 1752a, we don't re-use threads between profiles. Why is this something that we would need for the runtime case? It suggests that we have dependencies on context-local data that we should not have.

Do you think the concurrent processing of the identical AST by several clients using the same engine would be problematic?

It depends on what you mean by this. For source code conversion, it is something we must not have. But for dynamic queries, if the inputs are the same but they are processed completely independently, then there is no issue until you want to cache the results (assuming the cache is server-wide). Again, I would think we should avoid that.

Can you be more specific here? The current approach for runtime queries already does share pattern engine instances. I don't see anything in #1752 that describes an approach to use here.

Yes, the pattern engine is shared but it's not used in the same time. It's used sequentially by clients that process queries with the same profile. We could use the 1752a engine's ability to process multiple files at once for runtime conversion, but I'm no longer sure if that's a good fit.

In a non-runtime conversion using 1752a, we don't re-use threads between profiles. Why is this something that we would need for the runtime case? It suggests that we have dependencies on context-local data that we should not have.

That's right. I thought we should avoid generation of new threads and we could use those Conversation threads that already exist for runtime conversion.

It depends on what you mean by this. For source code conversion, it is something we must not have. But for dynamic queries, if the inputs are the same but they are processed completely independently, then there is no issue until you want to cache the results (assuming the cache is server-wide). Again, I would think we should avoid that.

I was only referring to runtime conversion. So, this would be a problematic case.

Should we continue with the current approach for now?

I like the idea that the conversation threads will process their own query/temp-table creation. This would avoid the creation of extra thread pools and would naturally ensure there are no bottlenecks (a session waiting for a thread to become available). So I do think this is preferred. It should also provide a performance improvement in high concurrency scenarios.

I assume this means that we still have 1 pattern engine instance per profile and we just rework it to allow the conversation threads to process as needed. In other words, the engine instances will be long lived and will remain one per profile.

We do need to properly serialize/synchronize anything shared, including identical ASTs. My biggest concern with this approach is whether there is an increased memory footprint due to each conversation thread having its own context local data. If this includes the schema dictionary, it might be a big hit indeed.

Greg Shah wrote:

I assume this means that we still have 1 pattern engine instance per profile and we just rework it to allow the conversation threads to process as needed. In other words, the engine instances will be long lived and will remain one per profile.

Exactly.

We do need to properly serialize/synchronize anything shared, including identical ASTs. My biggest concern with this approach is whether there is an increased memory footprint due to each conversation thread having its own context local data. If this includes the schema dictionary, it might be a big hit indeed.

Each conversation thread already holds its own context local data, including the SchemaDictionary. This is necessary because profiles use separate engine instances:

   /** Context-local SchemaDictionary instance. */
   private static final ContextLocal<SchemaDictionary> localDict = new ContextLocal<SchemaDictionary>()
   {
      @Override
      protected SchemaDictionary initialValue()
      {
         try
         {
            return new SchemaDictionary((Set<String>) null);
         }
         catch (SchemaException e)
         {
            throw new RuntimeException(e);
         }
      }
   };

As we discussed in yesterday's meeting, I've committed other changes that move callgraph execution into singlethreaded mode. The changes are committed in 1752a/16234.

• Could not find container for 2951 12884912449 [BLOCK id <12884912457> 0:0]

It seems to be just a PROCEDURE keyword with type KW_PROC, inside of a PROCEDURE AST. It's about activate-rooms-dialog.w.cache line 3260, to be more specific.

Is it PROCEDURE adm-clone-props :?

If you look in load_code_set.xml, descent-rules on ~190 will call push_container. But the walk-rules ~200 will create the graph node for it.

Try a simple .p with a procedure (non-empy) and see if the callgraph works. If it does, then add logging in load_code_set.xml to see why adm-clone-props is not being processed.

I suspect there is an issue with the CallGraph.getGraph() method. It is experiencing excessive runtime delays and eventually throws a timeout error. I've attached a relevant section of the log for review, as the recurring status warnings are concerning: callgraph_logcallgraph_log

[java] Some key(s) on index externalmissing do not currently have status(es) [REGISTERED]: external=ENABLED,missing=ENABLED
     [java] Some key(s) on index externalmissing do not currently have status(es) [REGISTERED]: external=ENABLED,missing=ENABLED
     [java] Some key(s) on index externalmissing do not currently have status(es) [REGISTERED]: external=ENABLED,missing=ENABLED
     [java] Some key(s) on index externalmissing do not currently have status(es) [REGISTERED]: external=ENABLED,missing=ENABLED
     [java] Some key(s) on index externalmissing do not currently have status(es) [REGISTERED]: external=ENABLED,missing=ENABLED
     [java] Some key(s) on index externalmissing do not currently have status(es) [REGISTERED]: external=ENABLED,missing=ENABLED
     [java] Some key(s) on index externalmissing do not currently have status(es) [REGISTERED]: external=ENABLED,missing=ENABLED
     [java] Timed out (PT1M) while waiting for index externalmissing to converge on status(es) [REGISTERED]
     [java] Index externalmissing metrics: success-tx: 1 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 2 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 3 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 4 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 5 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 6 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 7 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 8 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 9 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 10 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 11 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 12 doc-updates: 0 succeeded: 0
     [java] Index externalmissing metrics: success-tx: 13 doc-updates: 0 succeeded: 0
     [java] Index update job successful for [externalmissing]
     [java] All 2 key(s) on index externalmissing have status(es) [ENABLED]
     [java] Some key(s) on index procedurevirtual do not currently have status(es) [REGISTERED]: virtual=ENABLED,procedure=ENABLED

This issue is also occurring with a simple testcase that contains a non-empty procedure. Initially, I suspected an ant clean operation was necessary to prevent conflicts from previous runs, but the problem persists.

I noticed that the first processed index that generates Timed out (PT1M) is nodetypeappentrypoint. This index is created here, in CallGraphWorker.getGraph():

        GraphDB.createIndex(db,
                             new String[] {  "node-type", "app-entry-point" },
                             new Class<?>[] { Integer.class, Boolean.class },
                             true,  false);

Does this suggest that it could be a configuration issue related to the entry point that the graph wants to set? However, I set properly the entry point in cfg/rootlist.xml:

<?xml version="1.0"?>

<!-- Root Node List -->
<roots>

  <node filename="./cvt/start.p.ast" />
  <node filename="./cvt/emain.p.ast" />
  <node filename="./cvt/ehotel.p.ast" />

</roots>

It seems I had to add new individual indexes with other properties to avoid those Timed out errors:

=== modified file 'src/com/goldencode/p2j/uast/CallGraphWorker.java'
--- old/src/com/goldencode/p2j/uast/CallGraphWorker.java    2025-03-21 07:58:59 +0000
+++ new/src/com/goldencode/p2j/uast/CallGraphWorker.java    2025-10-24 11:45:50 +0000
@@ -281,6 +281,10 @@
          GraphDB.createIndex(db, "external",         Boolean.class, true,  false);
          GraphDB.createIndex(db, "included",         Boolean.class, true,  false);
          GraphDB.createIndex(db, "resource-domain",  Integer.class, true,  false);
+         GraphDB.createIndex(db, "app-entry-point",  Boolean.class, true,  false);
+         GraphDB.createIndex(db, "missing",          Boolean.class, true,  false);
+         GraphDB.createIndex(db, "virtual",          Boolean.class, true,  false);
+         GraphDB.createIndex(db, "entry-point",      Boolean.class, true,  false);

          GraphDB.createIndex(db,
                              new String[] { "node-id", "node-type"  },
@@ -322,6 +326,7 @@

This way, the problematic indexes are now successfully processed:

         GraphDB.createIndex(db,
                             new String[] {  "node-type", "app-entry-point" },
                             new Class<?>[] { Integer.class, Boolean.class },
                             true,  false);
         GraphDB.createIndex(db,
                             new String[] {  "external", "missing" },
                             new Class<?>[] { Boolean.class, Boolean.class },
                             true,  false);
         GraphDB.createIndex(db,
                             new String[] {  "procedure", "virtual" },
                             new Class<?>[] { String.class, Boolean.class },
                             true,  false);
         GraphDB.createIndex(db,
                             new String[] {  "function", "virtual" },
                             new Class<?>[] { String.class, Boolean.class },
                             true,  false);
         [...]

         for (int p : parents)
         {
            String prop = CallGraphHelper.parentProperty(p);

            GraphDB.createIndex(db, prop, Long.class, true, false);

            GraphDB.createIndex(db,
                                new String[] {  prop, "entry-point" },
                                new Class<?>[] { String.class, Boolean.class },
                                true,  false);
         }

Greg, Constantin, do you have any thoughts on this? Also, do you think we should solve this in a separate task?

I have no objection. It can be put the change into the current changes.

Constantin?

Greg Shah wrote:

I have no objection. It can be put the change into the current changes.

Constantin?

Current branch is OK.

I think the root cause with the callgraph generation is that the PROCEDURE ASTs are marked with hidden=true. This way, ast nodes for callgraph are not created, such as KW_PROC. The same for FUNCTION ASTs.

Octavian Adrian Gavril wrote:

I think the root cause with the callgraph generation is that the PROCEDURE ASTs are marked with hidden=true. This way, ast nodes for callgraph are not created, such as KW_PROC. The same for FUNCTION ASTs.

That should not be an issue. Those get marked hidden during conversion but not during callgraph. If they are marked hidden, then something is running (like annotations) that should not be running.

Greg Shah wrote:

That should not be an issue. Those get marked hidden during conversion but not during callgraph. If they are marked hidden, then something is running (like annotations) that should not be running.

Indeed, those are not marked hidden during the callgraph but the simple running of ant callgraph use the ASTs from the full conversion. So, the error mentioned in #3211-176 is avoided running ant clean callgraph. This way, the callgraph will reconvert the source files.

When I open the callgraph visualization, I get this type of errors in report-server.log:

report-server.logreport-server.log

applyActiveFile update count: 61
Entry point not found:./deploy/start.p
Entry point not found:./deploy/emain.p
Entry point not found:./deploy/ehotel.p
Entry point not found:./deploy/fwd-embedded-driver.p
Entry point not found:./deploy/ladder-safety-gui.p

java.lang.reflect.InvocationTargetException
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:569)
    at com.goldencode.p2j.report.server.ReportProtocol.processIncoming(ReportProtocol.java:401)
    at com.goldencode.p2j.report.server.ReportProtocol.onMessage(ReportProtocol.java:254)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:569)
    at org.eclipse.jetty.websocket.common.events.annotated.CallableMethod.call(CallableMethod.java:70)
    at org.eclipse.jetty.websocket.common.events.annotated.OptionalSessionCallableMethod.call(OptionalSessionCallableMethod.java:72)
    at org.eclipse.jetty.websocket.common.events.JettyAnnotatedEventDriver.onTextMessage(JettyAnnotatedEventDriver.java:301)
    at org.eclipse.jetty.websocket.common.message.SimpleTextMessage.messageComplete(SimpleTextMessage.java:69)
    at org.eclipse.jetty.websocket.common.events.AbstractEventDriver.appendMessage(AbstractEventDriver.java:67)
    at org.eclipse.jetty.websocket.common.events.JettyAnnotatedEventDriver.onTextFrame(JettyAnnotatedEventDriver.java:287)
    at org.eclipse.jetty.websocket.common.events.AbstractEventDriver.incomingFrame(AbstractEventDriver.java:152)
    at org.eclipse.jetty.websocket.common.WebSocketSession.incomingFrame(WebSocketSession.java:326)
    at org.eclipse.jetty.websocket.common.extensions.AbstractExtension.nextIncomingFrame(AbstractExtension.java:148)
    at org.eclipse.jetty.websocket.common.extensions.compress.PerMessageDeflateExtension.nextIncomingFrame(PerMessageDeflateExtension.java:111)
    at org.eclipse.jetty.websocket.common.extensions.compress.CompressExtension.forwardIncoming(CompressExtension.java:169)
    at org.eclipse.jetty.websocket.common.extensions.compress.PerMessageDeflateExtension.incomingFrame(PerMessageDeflateExtension.java:90)
    at org.eclipse.jetty.websocket.common.extensions.ExtensionStack.incomingFrame(ExtensionStack.java:202)
    at org.eclipse.jetty.websocket.common.Parser.notifyFrame(Parser.java:225)
    at org.eclipse.jetty.websocket.common.Parser.parseSingleFrame(Parser.java:259)
    at org.eclipse.jetty.websocket.common.io.AbstractWebSocketConnection.onFillable(AbstractWebSocketConnection.java:459)
    at org.eclipse.jetty.websocket.common.io.AbstractWebSocketConnection.onFillable(AbstractWebSocketConnection.java:440)
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105)
    at org.eclipse.jetty.io.ssl.SslConnection$DecryptedEndPoint.onFillable(SslConnection.java:555)
    at org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:410)
    at org.eclipse.jetty.io.ssl.SslConnection$2.succeeded(SslConnection.java:164)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105)
    at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131)
    at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:883)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1034)
    at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: com.goldencode.ast.AstException: Error loading AST XML file/resource: /home/aog/gcd/hotel_gui_orig/./cvt/server/abl/common/adecomm/as-utils.w.ast
    at com.goldencode.ast.XmlFilePlugin.loadTree(XmlFilePlugin.java:470)
    at com.goldencode.ast.AstManager.loadTree(AstManager.java:325)
    at com.goldencode.p2j.report.server.ReportApi.getSourceData(ReportApi.java:1254)
    ... 42 more
Caused by: java.lang.IllegalArgumentException: Missing/bogus input file/resource /home/aog/gcd/hotel_gui_orig/./cvt/server/abl/common/adecomm/as-utils.w.ast
    at com.goldencode.ast.XmlFilePlugin.loadTree(XmlFilePlugin.java:463)
    ... 44 more

Entry point not found:./deploy/start.p
Entry point not found:./deploy/emain.p
Entry point not found:./deploy/ehotel.p
Entry point not found:./deploy/fwd-embedded-driver.p
Entry point not found:./deploy/ladder-safety-gui.p
Entry point not found:./deploy/start.p
Entry point not found:./deploy/emain.p
Entry point not found:./deploy/ehotel.p
Entry point not found:./deploy/fwd-embedded-driver.p
Entry point not found:./deploy/ladder-safety-gui.p
Entry point not found:./deploy/start.p
Entry point not found:./deploy/emain.p
Entry point not found:./deploy/ehotel.p
Entry point not found:./deploy/fwd-embedded-driver.p
Entry point not found:./deploy/ladder-safety-gui.p
Entry point not found:./deploy/start.p
Entry point not found:./deploy/emain.p
Entry point not found:./deploy/ehotel.p
Entry point not found:./deploy/fwd-embedded-driver.p
Entry point not found:./deploy/ladder-safety-gui.p

The file paths seem broken.

Indeed, those are not marked hidden during the callgraph but the simple running of ant callgraph use the ASTs from the full conversion. So, the error mentioned in #3211-176 is avoided running ant clean callgraph. This way, the callgraph will reconvert the source files.

That won't work. You must only run callgraph on ASTs that are from a fresh run of the front-end. You can't reuse converted ASTs.

I think the reason why the file paths are broken is that here the absolute path is [p2j_home]/deploy/server/:

  public static String normalizeFilename(String home, String filename)
   {
      File   abnormal = new File(filename); 
      String absolute;

      try
      {
         // make best effort to clean up the path
         absolute = abnormal.getCanonicalPath();
      }
      catch (IOException exc)
      {
         // fallback plan if canonical doesn't work
         absolute = abnormal.getAbsolutePath();
      }
      

This was not an issue for full conversion because the paths are calculated when the absolute path is [p2_home]. Should we open a separate task for this issue?

If the problem is not caused by 1752a then it can be split off.

Greg Shah wrote:

If the problem is not caused by 1752a then it can be split off.

Yes, the issue is also on trunk/16242.

I've committed a last change that ensures singlethreaded execution of callgraph. The change is in 1752a/16235. I created #10784 for the issue with broken file path. To proceed with call graph testing, we first need to wait for #10784.

I continue experimenting new solutions for multithreaded runtime conversion.

I did this changes in ContextLocal$Fallback.get() in order to allow the null assignation from TRPL expressions of context local variables:

=== modified file 'src/com/goldencode/p2j/security/ContextLocal.java'
--- old/src/com/goldencode/p2j/security/ContextLocal.java    2024-09-03 11:00:43 +0000
+++ new/src/com/goldencode/p2j/security/ContextLocal.java    2025-10-31 11:11:19 +0000
@@ -934,10 +934,6 @@
        */
       public T get()
       {
-         if (value == null)
-         {
-            value = initialValue();
-         }
          return value;
       }

• I use get() when I specifically want to reset/initialize the variable.
• I use get(false) when I simply need the current value, without resetting it.

The full conversion process runs perfectly fine. However, during a runtime conversion, the variable JavaPatternWorker.context.root unexpectedly becomes null.

I cannot see the root cause for this. I suspect that a reset is being triggered somewhere due to my changes, but I don't understand the circumstances under which it occurs.

I'm trying to understand the purpose of duplicating the reset logic across both Fallback.get() and getImpl() (via the create flag). Why is it necessary to have the reset in both methods?

I think the changes in note 192 is not correct. Beside the conversion, ContextLocal objects are extensively used during runtime. All those occurrences expect the get() to return a non-null value! For getting the current value (possible null if not initialized), get(/*create=*/false) is to be used, as you noted there. If you want to (re-)set such object the set(T value) method is defined.

If JavaPatternWorker.context.root suddenly becoming null may be caused by the change of the context. Even if the context is static final, on each context it will hold a specific value, therefore accessing it from other thread will get you the value specific to that context.

The behaviour of these containers depends on the current mode (see MODE_* constants). It was optimised along FWD history. Even if the access operations seem fast, normal execution of FWD server requires millions of accesses making this a real bottleneck at runtime.

Ovidiu Maxiniuc wrote:

I think the changes in note 192 is not correct. Beside the conversion, ContextLocal objects are extensively used during runtime. All those occurrences expect the get() to return a non-null value! For getting the current value (possible null if not initialized), get(/*create=*/false) is to be used, as you noted there. If you want to (re-)set such object the set(T value) method is defined.

The current implementation is confusing because the create flag is checked only after the value has already been extracted from the fallback:

           value = (T)f.get();
            if (value == null && create) {
                value = (T)this.initialValue();
                this.set(value);
                this.initializeValue(value);
            }

The value is initialized regardless of whether the create flag is true or false, due to the absence of the changes noted in #3211-192.

The root nullify was triggered due to autoload flag which is set in the global rules. As my implementation call applyGlobal(RULE_INIT) just for the main thread, that flag was not visible (being true) for the other threads and the root was not initialized. I fixed that by sharing multiple static states as we do in AstProcessorJob:

   public void run()
   throws AstException
   {
      SymbolResolver.setLocal(sr);
      AstSymbolResolver.setLocal(resolver);
      JavaPatternWorker.setAutoload(autoload);
      AstManager.get().setPlugin(plugin);
      CommonAstSupport.setLocal(cas);

This way, the runtime conversion works with the new implementation but I found some regressions in the full conversion process. I continue investigating the cause of those issues, then I will test the solution.

I've committed new changes into 1752a/16236.

Initially, I wanted to avoid multiple calls of applyGlobal method over the same profile in order to improve performance of runtime conversion. This way, I decided to use context-local variables anytime for runtime conversion and made them non-thread-safe:

!Configuration.isRuntimeConfig() && VAL_TRUE.equals(threadSafe),

   <variable name="ifaceKeys"    type="java.util.List" />
   <variable name="classKeys"    type="java.util.List" />
   <variable name="rootRef"      type="com.goldencode.ast.Aast" />

   <!-- pipeline of rule-sets to process -->

   <!-- global initialization -->
   <init-rules>
      <rule>ifaceKeys = create("java.util.ArrayList")</rule>
      <rule>classKeys = create("java.util.ArrayList")</rule>
   </init-rules>

The current implementation is making sure that the values are copied by invoking each setValue call to all threads used in conversion process:

        if (!PatternEngine.singleThreadedMode && PatternEngine.refreshThreadData && isGlobal)
         {
            PatternEngine.addThreadsJob(() -> ((ContextLocal<Object>) this.value).set(deepCopyObject(value)));

The conclusion was that we cannot use the same design because the runtime conversion doesn't use PatternEngine.threads, so addThreadsJob would have no effect. That's why I had to refactor this design.

• Variable value managing:
  • First, the applyGlobal(RULE_INIT) is executed in a singlethreaded environment. All of the global variables are thread-safe during the execution of both PatternEngine.initialize() and PatternEngine.applyGlobal(RULE_INIT). This way, we allow complex operations to be done over the global variables and we execute these only once.
  • After those calls, we 'convert' all of the global AND non-thread-safe variables to Context-Local instances:

    +   public void convertToContextLocal()
    +   {
    +      Object currentValue = value;
    +      this.value = new ContextLocal<Object>()
    +      {
    +         @Override
    +         protected Object initialValue() {
    +            return deepCopyObject(currentValue);
    +         }
    +      };
        }

    Notice that we use the same deepCopyObject method in order to avoid sharing the same references (e.g. lists, maps etc.).
  • I added some changes in the ContextLocal class that should keep the same logic but allow null assignation using get(false). That's why, any Variable.getValue() will use get(false) in order to allow null assignation for non-thread-safe variables.
  • With all of this setup, the non-thread-safe variables will trigger the initialValue at the first access of the value. By 'access', I mean any call of Variable.setValue OR Variable.getValue.
• Sharing the engine's state:
  • So far, we shared any needed state of the engine through the constructor of AstProcessorJob (see #3211-195). I noticed that we need something similar for the runtime conversion as well. We don't use AstProcessorJob for runtime conversion so I had to implement a common design that can be used in both full and runtime conversions. I encapsulated all the properties we need in an inner class and use it when it's necessary.
• Renamed fields, added javadoc and other cosmetic changes.

Completed full conversion tests on hotel and ChUI regression testing apps. Next steps are testing other applications and investigating further runtime conversion improvements.

I tested the callgraph with 1752a. There were some issues related to reports generation. I moved that processing into singlethreaded mode as well. Now, the callgraph visualization works for hotel. I added also the changes mentioned in #3211-180 . I've committed the latest changes in 1752a/16237.

Octavian Adrian Gavril wrote:

I tested the callgraph with 1752a. There were some issues related to reports generation. I moved that processing into singlethreaded mode as well. Now, the callgraph visualization works for hotel. I added also the changes mentioned in #3211-180 . I've committed the latest changes in 1752a/16237.

Report generation should be able to be multithreaded. We don't want it to be restricted to single thred mode.

Octavian, please rebase 1752a - I'll review after that.

Constantin Asofiei wrote:

Octavian, please rebase 1752a - I'll review after that.

Rebased branch 1752a with trunk/16262. The new revision is 16315.

I fixed some rebase-related errors in rev16316.

• there are global variables in the cfg for a TRPL .xml - I don't understand how the decision was made to make a variable 'threadSafe' or not:
  • note that there are variables like typeMapping in schema/java_pojo.xml which are 'immutable collections' or constants. There is no need for them to be thread-safe.
  • other variables (like javaFile in schema/java_pojo.xml) are used in the walk-rules for a nested rule-set. This rule-set as I understand will be executed multi-threaded - this global variable is not marked as threadSafe. This poses problems as the TRPL code uses global variables in nested rule-sets, while these should have been declared and scoped to that rule-set. We will need to identify all these variables and either move them to the rule-set where it is used or mark them as threadSafe.
  • in annotations.xml there is a methodJavaNames global map - this used used by rule-sets which will be executed on multiple threads; so this map (and any other global collection) needs to be a concurrent collection.
  • I think we need to:
    • go through each .xml TRPL file and check each variable - if is used in non-global rules then it must be either made context-local or declared in that rule-set.
    • any var which needs to remain global must be i.e. a synchronized collection or otherwise access to it must be synchronized (note that threadSafe is kind of confusing - if it means context-local, then the name should be contextLocal; threadSafe to me indicates that access to it is safe in a concurrent mode, not that each thread has its own instance of the var).
• in p2j.convert.db package changes, to synchronize the code, you used a ReentrantLock - why not just make the method synchronized? This is the same effect with less churn on maintaining the code.

I'll continue looking throughout the other changes

Constantin Asofiei wrote:

Octavian, I'm looking through the code, but some things that stand out:

• there are global variables in the cfg for a TRPL .xml - I don't understand how the decision was made to make a variable 'threadSafe' or not:

threadSafe implies that a variable will not be context-local and its access is synchronized. The absence of the threadSafe attribute will make the value a ContextLocal instance.

• note that there are variables like typeMapping in schema/java_pojo.xml which are 'immutable collections' or constants. There is no need for them to be thread-safe.

I agree. But for these type of variables we can use threadSafe or context-local approach. The result will be the same. I'm not sure which one is more efficient because threadSafe needs synchronization and context-local requires copying the object of the main thread and create a new instance for all of the threads. What do you think?

• other variables (like javaFile in schema/java_pojo.xml) are used in the walk-rules for a nested rule-set. This rule-set as I understand will be executed multi-threaded - this global variable is not marked as threadSafe. This poses problems as the TRPL code uses global variables in nested rule-sets, while these should have been declared and scoped to that rule-set. We will need to identify all these variables and either move them to the rule-set where it is used or mark them as threadSafe.

The absence of the threadSafe attribute will make the value a ContextLocal instance. This way, each thread will have its 'own' value for javaFile and the TRPL code will be safely executed multi-threaded.

• in annotations.xml there is a methodJavaNames global map - this used used by rule-sets which will be executed on multiple threads; so this map (and any other global collection) needs to be a concurrent collection.
• I think we need to:

The same logic applies here as it did for the javaFile file in schema/java_pojo.xml. I've applied the threadSafe attribute to any variables whose data must be shared by different threads. This was initially necessary for handling the H2 data structures, which are designed to be shared. It was also used for certain global variables in global rules (like RULE_INIT and RULE_POST), but this should be fixed by the recent changes from 16314.

• in p2j.convert.db package changes, to synchronize the code, you used a ReentrantLock - why not just make the method synchronized? This is the same effect with less churn on maintaining the code.

That ReentrantLock is from an old initial approach. You're right, synchronized is cleaner and achieves the same thing. I've switched some classes already, but a few are still left with the lock. I'll get those updated to synchronized soon.

The current implementation from reports/consolidated_reports.xml is using mutable global variables in both nested rule-sets and <post-rules>. It seems to me that those are used to calculated a final result so they should be shared and marked as threadSafe. The problem is that we need atomic operations in order to safely synchronize those variables:

reports/consolidated_reports.xmlreports/consolidated_reports.xml

<cfg>

   <!-- register worker objects -->
   <worker class="com.goldencode.p2j.uast.ProgressPatternWorker"         namespace="prog" />
   <worker class="com.goldencode.p2j.report.ReportWorker"                namespace="rw" />
   <worker class="com.goldencode.p2j.preproc.PreprocessorHintsWorker"    namespace="preproc" />
   <worker class="com.goldencode.p2j.convert.ExpressionConversionWorker" namespace="ecw" />
   <worker class="com.goldencode.p2j.xml.XmlPatternWorker"               namespace="xml" />

   <!-- expression libraries -->
   <include name="common-progress" />
   <include name="report" />

   <!-- global variables -->
   [...]
   <variable name="totalLOC"        type="java.lang.Integer" />
   <variable name="totalDir"        type="java.lang.Integer" />
   <variable name="totalIncl"       type="java.lang.Integer" />
   <variable name="totalFiles"      type="java.lang.Integer" />
   <variable name="pathLOC"         type="java.lang.Integer" />
   <variable name="pathDir"         type="java.lang.Integer" />
   <variable name="pathIncl"        type="java.lang.Integer" />
   <variable name="pathFiles"       type="java.lang.Integer" />
   [...]

   <func-library access="private">

      [...]

      <function name="addRowByDir">
         <parameter name="outPath"  type="java.lang.String" />
         <variable  name="current"  type="java.util.LinkedList" />
         <variable name="percent"   type="java.lang.Double" />

         <rule>current = create("java.util.LinkedList")</rule>
         <rule>current.add(sprintf("%s (%d files)", outPath, pathFiles))</rule>
         <rule>current.add(pathLOC)</rule>
         <rule>pathDir = pathLOC - pathIncl</rule>
         <rule>current.add(pathDir)</rule>
         <rule>current.add((#(double) pathDir / #(double) pathLOC)))</rule>
         <rule>current.add(pathIncl)</rule>
         <rule>current.add((#(double) pathIncl / #(double) pathLOC)))</rule>
         <rule>rowsByDir.add(current)</rule>
      </function>

      [...]

   </func-library>

   <!-- global init -->
   <init-rules>

      <rule>databaseMode and !incremental
         <action>rw.initializeDatabase(appAdminPass)</action>
      </rule>

      <rule>rw.prepareDatabaseStatements(incremental)</rule>

      <rule>!databaseMode and !incremental

         <!-- lines of code report initialization -->

         <rule>basepath = getConfigParameter("basepath")</rule>
         <rule>basepath = sprintf("%s%s", basepath, fileSep)</rule>
         <rule>basesize = basepath.length()</rule>
         <rule>lastPath = ""</rule>

         <rule>totalLOC = 0</rule>
         <rule>totalDir = 0</rule>
         <rule>totalIncl = 0</rule>
         <rule>totalFiles = 0</rule>

         [...]

   </init-rules>

   <!-- processed once for each tree -->
   <rule-set input="tree" >

      <init-rules>

         [...]

         <rule>!databaseMode and !incremental

            <rule>last = -1</rule>
            <rule>num = 0</rule>
            <rule>dnum = 0</rule>
            <rule>inum = 0</rule>
            <rule>totalFiles = totalFiles + 1</rule>

            <action>curPath = execLib("get_current_path")</action>
            <rule>!lastPath.equals(curPath)

               <!-- output if this is not the first time through -->
               <rule>lastPath.length() &gt; 0
                  <!-- save our lastPath results -->
                  <action>execLib("addRowByDir", lastPath)</action>
               </rule>

               <!-- reset our counters for the new path -->
               <rule>lastPath = curPath</rule>
               <rule>pathLOC = 0</rule>
               <rule>pathDir = 0</rule>
               <rule>pathIncl = 0</rule>
               <rule>pathFiles = 0</rule>
            </rule>

         </rule>

      </init-rules>

      <post-rules>

         <!-- lines of code processing -->
         <rule>!databaseMode and !incremental

            <rule>totalLOC = totalLOC + num</rule>
            <rule>totalIncl = totalIncl + inum</rule>
            <rule>pathLOC = pathLOC + num</rule>
            <rule>pathIncl = pathIncl + inum</rule>
            <rule>pathFiles = pathFiles + 1</rule>

         [...]

         </rule>

         <rule>rw.postprocessFile()</rule>

      </post-rules>

   </rule-set>

   <!-- global post -->
   <post-rules>

      <!-- lines of code analysis -->
      <rule>!databaseMode and !incremental

         <!-- save our lastPath results -->
         <rule>execLib("addRowByDir", lastPath)</rule>           <-- see addRowByDir definition

         <!-- write a totals row into both reports -->
         <rule>current = create("java.util.LinkedList")</rule>
         <rule>current.add(sprintf("Totals (across %d files)", totalFiles))</rule>
         <rule>current.add(totalLOC)</rule>
         <rule>totalDir = totalLOC - totalIncl</rule>            <--

         [...]

   </post-rules>

</cfg>

To safely handle atomic operations on shared variables, such as pathLOC = pathLOC + num we can introduce a synchronized utility method:

public static void add(Variable var, Variable op)
{
   synchronized(var)
   {
      // do the operation using getValue() and setValue()
   }
}

The current implementation saves the results of the last processed path in the rule-set -> init-rules of the next path:

  <rule-set input="tree" >

      <init-rules>

         [...]

         <rule>!databaseMode and !incremental

            [...]

            <action>curPath = execLib("get_current_path")</action>
            <rule>!lastPath.equals(curPath)

               <!-- output if this is not the first time through -->
               <rule>lastPath.length() &gt; 0
                  <!-- save our lastPath results -->
                  <action>execLib("addRowByDir", lastPath)</action>
               </rule>

               <!-- reset our counters for the new path -->
               <rule>lastPath = curPath</rule>
               <rule>pathLOC = 0</rule>
               <rule>pathDir = 0</rule>
               <rule>pathIncl = 0</rule>
               <rule>pathFiles = 0</rule>
            </rule>

         </rule>

      </init-rules>

The problem with multi-threaded execution is that the results of the actual last path processed in reports generation are saved in the global post-rules:

  <!-- global post -->
   <post-rules>

      <!-- lines of code analysis -->
      <rule>!databaseMode and !incremental

         <!-- save our lastPath results -->
         <rule>execLib("addRowByDir", lastPath)</rule>

I have completed the implementation for multi-threaded report generation. Currently, I see no obvious improvement. While I initially suspected that excessive use of shared global variables was the root cause, I found that the primary constraint is the large number of conditional checks being executed:

      <walk-rules>
         <rule>iter = reports.iterator()</rule>

         <!-- create the list of matches -->
         <while>iter.hasNext()
            <action>rpt = iter.next()</action>

            <action>currentMatch = exec(rpt.condition)</action>

Synchronization is taking up too much time, so the improvements made are not noticeable.

I've committed new changes that allow report generation to be executed multi-threaded. I've also replaced the current locking system from Expression.getCompiledInstance with read/write locks. This way, the improvements are more noticeable.

These are the results with hotel and ChUI regression testing apps:

The changes are in 1752a/16317.

Octavian Adrian Gavril wrote:

Constantin Asofiei wrote:

• in p2j.convert.db package changes, to synchronize the code, you used a ReentrantLock - why not just make the method synchronized? This is the same effect with less churn on maintaining the code.

That ReentrantLock is from an old initial approach. You're right, synchronized is cleaner and achieves the same thing. I've switched some classes already, but a few are still left with the lock. I'll get those updated to synchronized soon.

The changes are now in rev 16318.

• The most utility is to protect the access over the shared collections but these have already synchronized operation. So, this implementation is redundant.
• Another utility is for immutable variables, that are initialized or processed in the global rules.

My initial approach was to use threadSafe less as possible because that flag implied synchronized access over the variable. But it seems that a more clear utility is to mark all the immutable variables and the collections that supports concurrency anyway, like those from p2j.convert.db package. This implies removing the current redundant synchronization.

This way, I expect to see improvements on both runtime and full conversion.

One more thing. The current design uses ContextLocal for all variables because the global post-rules are currently executed inside PatternEngine.run() (which runs per thread). To boost performance, we should look into using variables the same way the full conversion process does.

   public void run()
   {
      AstSymbolResolver.setLocal(resolver);
      WorkArea wa = context.get();

      try
      {
         resolver.setRuleScope(this);

         int size = wa.targetAsts.size();
         // Process each in-memory AST
         for (int i = 0; i < size; i++)
         {
            Aast targetAst = wa.targetAsts.get(i);
            processAst(targetAst);
         }

         resolver.setRuleScope(this);
         // run global post rules.
         applyGlobal(RULE_POST);
      }
   [...]

I successfully moved applyGlobal(RULE_INIT) out of the run() method, but moving applyGlobal(RULE_POST) is proving to be a bit difficult.

Maybe we can simplify things by letting only those variables used in the post-rules be considered ContextLocal when we execute runtime conversion.

Since some profiles already ignore rules during runtime conversion, I'm wondering if the @applyGlobal(RULE_POST); call in PatternEngine.run() is actually useful or if we can safely delete it. What do you think?

Global post rules must not run on multiple threads. The threads for a given phase should all complete and then a single thread should run the global post, just like we do for the global init.

the collections that supports concurrency anyway, like those from p2j.convert.db package. This implies removing the current redundant synchronization.

Be careful here. The concurrent collections are not 100% thread-safe. The simple access actions (get/add/put/remove...) are protected from simultaneous access, but any object contained in the collection is mutable. This can lead to very dangerous results where multiple threads are referencing the same object instance thinking it is safe to modify that instance and instead we get data corruption or a "last to write wins" situation. In this case, we must make sure that once we read a reference out of the collection, that we lock that reference for the during of our usage (not just edits).

Greg Shah wrote:

Global post rules must not run on multiple threads. The threads for a given phase should all complete and then a single thread should run the global post, just like we do for the global init.

I agree, but the implementation from trunk is applying global init and global post for each runtime query. By sharing the engine between Conversation threads, I could successfully run applyGlobal(RULE_INIT) once, when the engine is created. But I can't do the same for RULE_POST, as this would imply that all the threads which use the same engine should finish their job and then run applyGlobal(RULE_POST) on single-threaded. I don't think this is what we want for multi-threaded runtime conversion.

In this case, I'm not sure that we have a proper place to run applyGlobal(RULE_POST) on single-threaded.

For the runtime case, I think we have to consider each session to be a separate conversion run with just a single thread. That would mean both global init and global post have to run on the session thread. Another way to think about it is that the session-level runtime conversion must be an independent conversion from start to finish. I think that implies that we have to have each session essentially run on its own thread and pattern engine instance. How does that track with the current implementation?

I think the behavior of TRPL is incorrect if we don't do that. I suspect some of the trouble you are dealing with is from this problem.

For non-runtime mode, the global post needs to be in a single thread after all other threads finish.

Greg Shah wrote:

For the runtime case, I think we have to consider each session to be a separate conversion run with just a single thread. That would mean both global init and global post have to run on the session thread. Another way to think about it is that the session-level runtime conversion must be an independent conversion from start to finish. I think that implies that we have to have each session essentially run on its own thread and pattern engine instance. How does that track with the current implementation?

In the current implementation from trunk, the same engine is used for the same profile but we run conversions thread by thread (synchronizing the engine pool). By giving a separate engine instance for each session, we almost develop the current idea marked as TODO in trunk (please see #3211-168). The difference will be that the number of engines will be dynamic and not hard-coded. With this implementation, the changes for multi-threaded conversion are useless. This way, we should mark all as being threadSafe for runtime conversion.

For non-runtime mode, the global post needs to be in a single thread after all other threads finish.

Yes, this logic is implemented in 1752a in the PatternEngine.run(String profile) method.

• I applied threadSafe to all of the global read-only variables. I went through all .xml files and checked the immutable variables which have an initializer or are initialized in the global init-rules. The synchronized H2 collections are still marked as threadSafe but this should be happening just for full conversion because when we run runtime conversion these are simple HashMap s. I would consider to add a different label for these variables (like H2Collection or persistentCollection, etc.). What do you think?
  • Another thought I have is that we could disable setValue calls for global read-only variables. Just to be sure that the convention is preserved. This would implies that we mark differently global read-only vars than H2 collections.
• I removed unused TRPL variables and did some clean up. I checked for usages of those and safely removed them.
• Removed synchronization from Compiler class. This is used only once per expression. We don't compile the same expression twice and we already synchronized the access to the cached compiled instance in getCompiledInstance().
• Removed the system responsible triggering the initialValue from the rev16317 that involved usage of a ContextLocal flag to monitor the variable access per thread. Instead, I added initialization for those variables before processing rule-sets, just once per thread.
• I refactored some ContextLocal usages from several workers to avoid multiple calls of .get() method.

I tested the changes for full conversion and the improvements are noticeable. I did the same for runtime conversion and the results are better but not enough. The main problem remains that all global variables have ContextLocal value during runtime conversion. As we discussed in the previous notes, this happens because of applyGlobal(RULE_POST) call from PatternEngine.run() which is thread-specific.

So, we should decide how we handle engine-related logic of runtime conversion.

Greg Shah wrote:

the collections that supports concurrency anyway, like those from p2j.convert.db package. This implies removing the current redundant synchronization.

Be careful here. The concurrent collections are not 100% thread-safe. The simple access actions (get/add/put/remove...) are protected from simultaneous access, but any object contained in the collection is mutable. This can lead to very dangerous results where multiple threads are referencing the same object instance thinking it is safe to modify that instance and instead we get data corruption or a "last to write wins" situation. In this case, we must make sure that once we read a reference out of the collection, that we lock that reference for the during of our usage (not just edits).

I analyzed this aspect. The referenced objects from an H2 collection could be String, Int, Long (which are immutable) or another H2 collection (using synchronized operations). This way, the only scenario we should be aware is when there are non-thread-safe pairs of access actions over the same H2 collection (get + put, remove + get, etc). We handled these cases by turning them into atomic operations (please see #3211-83) or adding locks where we had no choice (e.g. p2o.xml processing). I will do a recheck to be sure that we handled all cases in the last revision.

I fixed some regressions caused by rev16319. The changes are in rev16320.

Octavian Adrian Gavril wrote:

I tested the changes for full conversion and the improvements are noticeable. I did the same for runtime conversion and the results are better but not enough. The main problem remains that all global variables have ContextLocal value during runtime conversion. As we discussed in the previous notes, this happens because of applyGlobal(RULE_POST) call from PatternEngine.run() which is thread-specific.

So, we should decide how we handle engine-related logic of runtime conversion.

Greg, Constantin, how should we proceed in this case?

• please rebase
• rules/progress/*.xml - does this need to be changed for multi-threaded mode?
• rules/annotations/legacy_services.xml - if not changed, then this will work only in single-threaded mode. See GenerateLegacyProxyOpenClient.java; if so, document in the class javadoc and in the .xml file that this is meant to be single-thread.
• lots of introduced ContextLocal usage for TRPL variable work - we need to measure the performance impact at runtime.
• there is a dead-lock for "waiting on the Class initialization monitor" for Dialect and P2JPostgreSQLDialect() - tried to convert something with two threads and couldn't. These dialect classes need to be initialized from the main thread (somehow), or just initialize the cvtdb from the main thread, in ConversionDriver before the front phase?
• what testing has been done until now? report generation, conversion,etc
• rules/callgraph/reports.xml - no actual change, please revert the file
• rules/reports/simple_search.xml - if you run this in multi-threaded mode, rows and nextId need to be threadSafe. But PatternEngine.main has no knowledge of 'numThreads' - do we need to add it?
• CopyOnWriteArrayList usage - this is supposed to be used for read-many/write-less cases;
  • in rules/reports/consolidated_reports.xml, this is not the case - the read is done only in global post-rules; I expect lots of 'trashing' from garbage collector when generating reports.
• AnnotatedAst - annotations was changed from IdentityHashMap to a LinkedHashMap: these strings are intern'ed, and this was a performance decision to use IdentityHashMap. Please consider changing back.
• XmlFilePlugin.saveTree - is it possible for the same AST file to be saved from multiple threads? If so:
  • what would be the cases?
  • what's the confidence that the AST is the same in both threads?
• expr.Expression method getCompiledInstance(): this will end up creating different CompiledExpression compiled instances for the same expression like type == prog.kw_funct, with the same SymbolResolver. Only one will be cached; how OK is this, for two threads to use different CompiledExpression instances?
• expr.Variable
  • has dependency on PatternEngine to determine if now we are creating the global vars; some of them will be context-local, I don't understand why we can't create them directly as ContextLocal when setValue is first called. Or better said, save the initializer and call it in each context's initialValue.
  • I do not like deepCopyObject. This needs to be improved (or even removed, see previous point), if the value is not Serializable but is mutable, then it will be shared between threads, even if is context-local.
  • isFirstAccess is never used
  • this is important - an assignment operation for a threadSafe variable is not atomic; you can have in TRPL someVar = create("..CopyOnWriteArrayList"), which can execute from multiple threads, and thread T1 can end up working with REF1 of the var, until thread 2 creates REF2; and REF1 gets lost. Do we have checks that a threadSafe var can be initialized only and only in the global pipeline init rules? If not, this needs to be added, with a catastrofic failure if this happens.
• expr.SymbolResolver
  • dependency on AstSymbolResolver.getResolver().getPatternEngine() - why not mark the PatternEngine via some flag that this is what you need here, and call Scope.isAutoRelease()?
  • for AstKey and FrameAstKey dependency, add a expr.Resettable interface and just check via 'instanceof' and call ((Resettable) var).reset()
  • resolveFunction two methods - avoid calling currentScope.get() twice
• ConversionDriver
  • remove P2JPostgreSQLDialect import
• AstSymbolResolver.java
  • reduce the usage of context.get()
• CommonAstSupport.java
  • there are some unneded locks imports
  • why is keepPathsWithSameFilenames() needed?
• PatternEngine.java
  • use star import, and not individual classes (this is for other files, too)
  • triggerVars(), addThreadsJob are missing javadoc
• ReportWorker.java
  • reduce the context-local usage
• AstKey and FrameAstKey
  • when is the LOCKS_CACHE having keys removed? Is this used by runtime - if so, then it will grow forever.
• missing history entry:
  

  rules/schema/generate_ddl.xml
  rules/annotations/annotations_prep.xml
  rules/callgraph/generate_call_graph.xml
  rules/callgraph/list_edges.xml
  rules/callgraph/load_code_set.xml
  rules/convert/oo_calls.rules
  rules/dbref/apply_refs.xml
  rules/convert/proxy_programs.xml
  rules/include/i18n.rules
  rules/reports/file_filter.xml
  rules/reports/generic_variable_usage_report.xml
  rules/reports/list_classes.xml
  rules/reports/pphints_check.xml
  rules/reports/rpt_file_list.xml
  rules/reports/rpt_nullable_vars.xml
  rules/reports/rpt_run_stmt_file_invocation.xml
  rules/reports/simple_search.xml
  rules/schema/annotations.xml
  rules/schema/data_analyzer.xml
  rules/schema/generate_seq_ddl.xml
  rules/schema/import.xml
  rules/schema/reindex.xml
  rules/schema/rpt_data_length.xml
  rules/schema/word_reindex.xml
  rules/unreachable/unreachable.xml
  src/com/goldencode/expr/Scope.java
  src/com/goldencode/p2j/convert/NameConverterWorker.java
  

• missing javadocs:
  

  PatternEngine.state
  NameConverter$WorkArea
  NamedFunction$WorkArea
  

The main problem remains that all global variables have ContextLocal value during runtime conversion.

I don't think this can be changed, and 'threadSafe' can't be true for runtime conversion. We can consider runtime conversion as running conversions in parallel in the same JVM, for different applications, which is not the same as our multi-threaded mode for a single application.

Rebased branch 1752a with trunk/16354. The new revision is 16412. I added some fixes for post-rebase errors in revision 16413.

Constantin Asofiei wrote:

This is the review for 1752a rev 16320:

• rules/annotations/legacy_services.xml - if not changed, then this will work only in single-threaded mode. See GenerateLegacyProxyOpenClient.java; if so, document in the class javadoc and in the .xml file that this is meant to be single-thread.
• rules/callgraph/reports.xml - no actual change, please revert the file
• expr.SymbolResolver
  • dependency on AstSymbolResolver.getResolver().getPatternEngine() - why not mark the PatternEngine via some flag that this is what you need here, and call Scope.isAutoRelease()?
  • for AstKey and FrameAstKey dependency, add a expr.Resettable interface and just check via 'instanceof' and call ((Resettable) var).reset()
  • resolveFunction two methods - avoid calling currentScope.get() twice
• AstSymbolResolver.java
  • reduce the usage of context.get()
• CommonAstSupport.java
  • there are some unneded locks imports
• PatternEngine.java
  • use star import, and not individual classes (this is for other files, too)
  • triggerVars(), addThreadsJob are missing javadoc
• AstKey and FrameAstKey
  • when is the LOCKS_CACHE having keys removed? Is this used by runtime - if so, then it will grow forever.
• missing history entry:
  [...]
• missing javadocs:
  [...]

These are fixed in rev16414.

Hi Octavian, this is for 1752a/16414.

• there is a dead-lock for "waiting on the Class initialization monitor" for Dialect and P2JPostgreSQLDialect() - tried to convert something with two threads and couldn't. These dialect classes need to be initialized from the main thread (somehow), or just initialize the cvtdb from the main thread, in ConversionDriver before the front phase?

I found some usages of DialectHelper in NameConverter but I'm not sure if that's related. Do you have any testcase or code areas that could be related?

Just try to convert a simple file with message "test". and -numthreads=2 - there is a deadlock.

• what testing has been done until now? report generation, conversion,etc

Report generation was tested for Hotel GUI and ChUI regression testing app, as far as I recall. About conversion, I tested Hotel GUI, ChUI regression testing app, an OO application and a large GUI application.

Was this only single-threaded? We need to evaluate how multi-threaded works, and how stable/deterministic the conversion is.

• AnnotatedAst - annotations was changed from IdentityHashMap to a LinkedHashMap: these strings are intern'ed, and this was a performance decision to use IdentityHashMap. Please consider changing back.

I agree. I used LinkedHashMap to make comparison process between ASTs much easier.

OK, please plan to change it back at some point.

• XmlFilePlugin.saveTree - is it possible for the same AST file to be saved from multiple threads? If so:
  • what would be the cases?
  • what's the confidence that the AST is the same in both threads?

No, it shouldn't happen. Each AST file is processed exclusively by a single thread. About DMOs or frames, a single thread is delegated to persist its changes to a certain AST.

Then why the need for locking?

• expr.Expression method getCompiledInstance(): this will end up creating different CompiledExpression compiled instances for the same expression like type == prog.kw_funct, with the same SymbolResolver. Only one will be cached; how OK is this, for two threads to use different CompiledExpression instances?

I don't think this is what is happening. All threads share the same SymbolResolver instance which uses instCache for caching compiled expressions.

I've debugged this and it can happen; the cache is on the expression's infix+flags. If you want to have a consistent cache, the lock must include both the read and write operations.

• expr.Variable
  • has dependency on PatternEngine to determine if now we are creating the global vars; some of them will be context-local, I don't understand why we can't create them directly as ContextLocal when setValue is first called. Or better said, save the initializer and call it in each context's initialValue.

The current implementation is designed to allow performing complex operations to calculate the value of the global variables. setValue could be called many times for the same variable during global rules. So, the "final" version is done at the end of global init-rules (it will be propagated to all threads). That's why we convert all non-thread-safe variable to context-local just after global initialization is done. This way, we avoid calculation of the same value.

Are you referring here to the need of deepCopyObject and because applyGlobal(RULE_INIT); is executed from the main thread, always? Then I think we need to abend if deepCopyObject is not guaranteed to return an immutable instance (i.e. value is not Serializable or a Java type). Maybe we can also consider Cloneable instances.

• I do not like deepCopyObject. This needs to be improved (or even removed, see previous point), if the value is not Serializable but is mutable, then it will be shared between threads, even if is context-local.
• this is important - an assignment operation for a threadSafe variable is not atomic; you can have in TRPL someVar = create("..CopyOnWriteArrayList"), which can execute from multiple threads, and thread T1 can end up working with REF1 of the var, until thread 2 creates REF2; and REF1 gets lost. Do we have checks that a threadSafe var can be initialized only and only in the global pipeline init rules? If not, this needs to be added, with a catastrofic failure if this happens.

I totally agree. This was a significant condition when I decided which variable can be considered thread-safe or not. I proposed something similar in the first point from #3211-216.

• the setter for threadSafe vars must not be allowed when PatternEngine.isInitializingGlobals is false - i.e. if this happens, then abend.
• isInitializingGlobals needs to be vollatile
• remove the dependency on PatternEngine from expr.Variable
• only global vars can be thread-safe; you can't have a thread-safe var which is not global - please add a protection for this.

• CommonAstSupport.java
  • why is keepPathsWithSameFilenames() needed?

There was an issue with files that have same name. We usually add a suffix for such cases. a.p and a.w end up like A.java and A_1.java. Moving to multi-threaded introduced non-deterministic names generation like the DMOs issue. I had to generate names in a separate single-threaded conversion phase (Compute class names). But I wanted to do it only for problematic filenames. So, keepPathsWithSameFilenames is responsible for checking if this processing is needed.

What about a_1.p and a-1.p - does this work? Here I mean when the converted Java name is the same, while 4GL names are different, and FWD must add a suffix.

• AstKey and FrameAstKey
  • when is the LOCKS_CACHE having keys removed? Is this used by runtime - if so, then it will grow forever.

I think AstKey and FrameAstKey are used exclusively for conversion to share the same DMO/frame AST definition. I could add a cleanUp method, just to be sure.

Yes, the cache needs to be cleared.

In other words, we cannot use shared global variables because the data is context-specific to each Conversation thread. While global sharing might offer a minor performance gain, it is semantically incorrect because the state is not meant to be duplicated or accessed across different thread boundaries. Is that right?

Correct, the state is not meant to be shared between threads. With trunk, we don't allow the same ConversionProfile to be ran in parallel, so the 'global vars' are not an issue. For runtime conversion, we need to assume all global vars are context-local (i.e. thread-safe is set to false, always). Please check how this can be done.

• lots of introduced ContextLocal usage for TRPL variable work - we need to measure the performance impact at runtime.
• rules/reports/simple_search.xml - if you run this in multi-threaded mode, rows and nextId need to be threadSafe. But PatternEngine.main has no knowledge of 'numThreads' - do we need to add it?
• CopyOnWriteArrayList usage - this is supposed to be used for read-many/write-less cases;
  • in rules/reports/consolidated_reports.xml, this is not the case - the read is done only in global post-rules; I expect lots of 'trashing' from garbage collector when generating reports.
• AstSymbolResolver.java
  • reduce the usage of context.get()
• ReportWorker.java
  • reduce the context-local usage

Constantin Asofiei wrote:

Hi Octavian, this is for 1752a/16414.

I found some usages of DialectHelper in NameConverter but I'm not sure if that's related. Do you have any testcase or code areas that could be related?

Just try to convert a simple file with message "test". and -numthreads=2 - there is a deadlock.

• expr.Expression method getCompiledInstance(): this will end up creating different CompiledExpression compiled instances for the same expression like type == prog.kw_funct, with the same SymbolResolver. Only one will be cached; how OK is this, for two threads to use different CompiledExpression instances?

I don't think this is what is happening. All threads share the same SymbolResolver instance which uses instCache for caching compiled expressions.

I've debugged this and it can happen; the cache is on the expression's infix+flags. If you want to have a consistent cache, the lock must include both the read and write operations.

I need to keep investigating these items.

• what testing has been done until now? report generation, conversion,etc

Report generation was tested for Hotel GUI and ChUI regression testing app, as far as I recall. About conversion, I tested Hotel GUI, ChUI regression testing app, an OO application and a large GUI application.

Was this only single-threaded? We need to evaluate how multi-threaded works, and how stable/deterministic the conversion is.

• Hotel GUI works fine with numThreads=6;
• applications exceeding the scale of the ChUI regression testing app, or of equivalent size, perform optimally with numThreads=16;
• for smaller conversions, a configuration of numThreads=2 is recommended.

Version 1752a includes cumulative fixes for non-deterministic behavior found in recent project conversions. Although we expect these issues to be resolved, a full conversion testing is necessary to ensure stability.

I agree. I used LinkedHashMap to make comparison process between ASTs much easier.

OK, please plan to change it back at some point.

Sure!

• XmlFilePlugin.saveTree - is it possible for the same AST file to be saved from multiple threads? If so:
  • what would be the cases?

No, it shouldn't happen. Each AST file is processed exclusively by a single thread. About DMOs or frames, a single thread is delegated to persist its changes to a certain AST.

Then why the need for locking?

Please excuse the confusion. I recall that indeed, it is possible for the same AST file to be saved from multiple threads. That's the case when two classes inherit the same super-class. There was some processing that added annotations to referenced variables if I recall correctly. I documented this problem for first time in #3211-85. The point is that in FWD we allow this and there wasn't a specific order of the 'child' classes. So, I considered that multi-threaded should do the same but in a synchronized way.

what's the confidence that the AST is the same in both threads?

I thought this issue was fixed, but I'm not so sure anymore.

The current implementation is designed to allow performing complex operations to calculate the value of the global variables. setValue could be called many times for the same variable during global rules. So, the "final" version is done at the end of global init-rules (it will be propagated to all threads). That's why we convert all non-thread-safe variable to context-local just after global initialization is done. This way, we avoid calculation of the same value.

applyGlobal(RULE_INIT); is executed from the main thread, always?

That's true.

Then I think we need to abend if deepCopyObject is not guaranteed to return an immutable instance (i.e. value is not Serializable or a Java type). Maybe we can also consider Cloneable instances.

We can rework this then.

Lets take a step back about the race condition; am I correct to assume that applyGlobal(RULE_INIT); is always called on the main thread? If so, then there is no race condition; the only things that remain are:

• the setter for threadSafe vars must not be allowed when PatternEngine.isInitializingGlobals is false - i.e. if this happens, then abend.
• isInitializingGlobals needs to be vollatile
• remove the dependency on PatternEngine from expr.Variable
• only global vars can be thread-safe; you can't have a thread-safe var which is not global - please add a protection for this.

I agree. I will implement these items.

What about a_1.p and a-1.p - does this work? Here I mean when the converted Java name is the same, while 4GL names are different, and FWD must add a suffix.

This case is handled because keepPathsWithSameFilenames groups all paths by their converted filename using getConvertedFilenameWithoutExtension.

What about these previous points?

• lots of introduced ContextLocal usage for TRPL variable work - we need to measure the performance impact at runtime.

I see. We can achieve the same behavior/performance with the trunk runtime conversion by defaulting variables to threadSafe=true. Given that pattern engines are unshared, thread contention is impossible. To improve the semantic clarity of this logic, we can replace boolean with Boolean, using null as a specific marker for runtime mode, distinct from the explicit true or false settings used in multi-threading.

• rules/reports/simple_search.xml - if you run this in multi-threaded mode, rows and nextId need to be threadSafe. But PatternEngine.main has no knowledge of 'numThreads' - do we need to add it?

I checked again the rules files responsible for report generation and it seems that only reports/consolidated_reports.xml is used. I didn't find any usages for reports/*.xml. So, I reverted all changes from 1752a. (in 1752a/16415)

• CopyOnWriteArrayList usage - this is supposed to be used for read-many/write-less cases;
  • in rules/reports/consolidated_reports.xml, this is not the case - the read is done only in global post-rules; I expect lots of 'trashing' from garbage collector when generating reports.

I replaced that with a function that creates Collections.synchronizedList(new ArrayList<>()) in order to support synchronized add operation inside walk-rules. (in 1752a/16415)

• AstKey and FrameAstKey
  • when is the LOCKS_CACHE having keys removed? Is this used by runtime - if so, then it will grow forever.

I think AstKey and FrameAstKey are used exclusively for conversion to share the same DMO/frame AST definition. I could add a cleanUp method, just to be sure.

Yes, the cache needs to be cleared.

What about these previous points?

• AstSymbolResolver.java
  • reduce the usage of context.get()
• ReportWorker.java
  • reduce the context-local usage

These are committed in 1752a/16415.

Give me a few hours to document some thoughts. I think we need to make a design change related to how we use context-local variables. The problems I see might explain some level of non-determinism.

Octavian Adrian Gavril wrote:

No, I tested conversion of those projects with several threads number and found for each one the optimal number:

Do you remember what improvement in the conversion time this gets us? I mean from 10h to 5h or something like this.

Constantin Asofiei wrote:

Octavian Adrian Gavril wrote:

No, I tested conversion of those projects with several threads number and found for each one the optimal number:

Do you remember what improvement in the conversion time this gets us? I mean from 10h to 5h or something like this.

• ChUI regression testing app from ~40 minutes to ~20 minutes
• OO application from ~1h and 20 minutes to 40 minutes
• large GUI application from ~9h and 45 minutes to ~5h and 10 minutes.

So, we have almost 50% time improvement for all of these projects

Octavian Adrian Gavril wrote:

What about these previous points?

• lots of introduced ContextLocal usage for TRPL variable work - we need to measure the performance impact at runtime.

I see. We can achieve the same behavior/performance with the trunk runtime conversion by defaulting variables to threadSafe=true. Given that pattern engines are unshared, thread contention is impossible. To improve the semantic clarity of this logic, we can replace boolean with Boolean, using null as a specific marker for runtime mode, distinct from the explicit true or false settings used in multi-threading.

I don't mean to assume threadSafe=true, but threadSafe=false, always, for runtime conversion. Otherwise we can't allow same .xml TRPL to be executed by multiple threads at once.

Code Review Task Branch 1752a Revisions 16263 through 16320 (this is the code base before the rebase from #3211-221)

Sorry it has taken me so long to put these concerns into writing. Some of it was just me needing time to review the code and think about it.

1. Too much stuff is shared across threads.

I think the usage of ContextLocal (in our TRPL/PatternEngine/compiled expressions implementation) is the wrong approach. I have 3 issues with this:

1. ContextLocal is expensive, since it involves (at least) 2 map lookups on every variable access. This is costly and must be slowing us down.
2. It mostly works which then leads us to believe that our "shared instance" approach is OK. This is flawed, see bug below).
3. It creates dependencies from the core pattern and expression packages to the security package in P2J. Considering that TPRL and the compiled expressions are not intended to be P2J-specific, these is not wanted. This last issue is not a "deal breaker" but it makes the approach less attractive.

Bug I think the core problem with our current design is that we have a single instance of the CompiledExpression and single instances of Scope which are shared for all threads. That is an inherently flawed design because that same compiled expressions can be executing on multiple threads at the same time. When that happens, it is possible for calls to Variable (e.g. Variable.getValue()) to be made from one thread and then the result to be accessed on another thread (because of a thread context switch). I certainly see that the chances are low in most cases, but the chances are not zero. It is possible for this to occur and it may be the cause of some non-determinism we've seen previously. I wonder if some of the parts of our code in which we force single threading are linked to this issue.

I suspect that we need to split the Scope instances out with separate copies per thread and I am sure that the CompiledExpression instances MUST BE per thread. Although we could try to add more syncrhonization around the expression usage to eliminate this race condition, it would add complexity and overhead into a solution that is already not correct. I don't want to go down that path.

Here is what I suggest to fix this, which also has the good effect of eliminating the ContextLocal usage:

• Compiler.process() currently generates the class AND also calls instantiate(). This means that we only create one instance of the class per compile. We need to split that up. instantiate() should not be called from process(), instead it should be called at first access from a new thread.
• During instantiate(), we copy the libraries and variables. This is the moment that we can make a deep copy of the variable to create a dedicated instance of each variable for each thread. We need to find the right place to call instantiate().
• I am assuming that we have already made all libraries thread safe. If we have not done that, we need to do so. The idea is that any library method that is called will handle any synchronization needed to access or modify the shared data of the library. With this in mind, we can shared instances of the library. The problem I see is for data that needs to be thread local (e.g. how we contain/manipulate references to AST nodes in libraries). A solution would be to keep that as instance data and make instances of the libraries too.
• I think that we need for all classes that descend from Scope to be instantiated per-thread. That will ensure that the maps used to track resources per-scope would be managed on a per-thread basis. I suspect some of the ScopeState could be safely shared but I know that the variable instances are not part of that.

2. I agree with Constantin that the use of "threadSafe" is confusing. The normal use of the term means that something is safe for multi-thread access. In our usage here, it means "we will implement synchronization to ensure that this reference is only accessed by one thread at a time". The usage is close but can be confusing. Perhaps we rename this "multiThreadAccess" instead?

3. In multiple places, the TRPL code now uses putIfAbsent(). For example, qualifiedOONames.putIfAbsent(name, create("java.util.TreeSet")) in collect_oo.rules. The problem here is that the code now unconditionally executes the code (in this case create("java.util.TreeSet")) which is very inefficient. The older code only executed this if needed. Since TRPL doesn't directly support lambdas, we should take a different approach. I think this is a place where we need a helper class that implements thread safe operations to replace the original code.

4. Are we treating the H2Map/H2Set/LoggedCollection as threadsafe? I don't see any logic in those classes that suggests they are threadsafe. All edits and access are unprotected.

5. It is hard to reason about the collection variables (maps, sets) marked as "threadsafe". I think for each of those definitions, we need a comment explicitly added which explains why the contents are safe for multithread access.

6. In com.goldencode.expr.SymbolResolver.resetVariables(), there should be no PatternEngine and other references to things in the p2j packages. The fact that we need such references during reset suggests we have something elemental that is not correct in our design.

7. It is not clear to me why access to com.goldencode.expr.SymbolResolver.instCache is no longer protected by locking.

Greg Shah wrote:

Bug I think the core problem with our current design is that we have a single instance of the CompiledExpression and single instances of Scope which are shared for all threads.

Greg, I don't think this is correct; the CompiledExpression is stateless - this is how it looks for i.e. i = i + 1 expression:

package com.goldencode.expr;

import com.goldencode.expr.CompiledExpression;

public class CE10
extends CompiledExpression {
    public Object execute() {
        this.setVar(0, (Object)new Integer((Integer)this.getVar(0) + 1));
        return null;
    }
}

The state is kept via expr.Variable, which in 1752a was changed to allow ContextLocal support. So I don't see how another thread can end up changing the value before the assignment completes.

But I agree that ContextLocal is too aggressive. I chose this approach initially if we need any context-state for runtime conversion, from the TRPL conversion engine like the 'pattern' classes, SchemaDictionary, etc. But from expr package, what we keep there it matters only to the thread - no other thread can access it. So I wonder if from inside expr package, we can maybe switch to ThreadLocal

Also, for runtime conversion, I would like to avoid creating 100k's of CompiledExpression instances, which are short-lived; but this I think needs to be considered with the cost of ThreadLocal access...

Greg Shah wrote:

Code Review Task Branch 1752a Revisions 16263 through 16320 (this is the code base before the rebase from #3211-221)

Sorry it has taken me so long to put these concerns into writing. Some of it was just me needing time to review the code and think about it.

1. Too much stuff is shared across threads.

I think the usage of ContextLocal (in our TRPL/PatternEngine/compiled expressions implementation) is the wrong approach. I have 3 issues with this:

1. ContextLocal is expensive, since it involves (at least) 2 map lookups on every variable access. This is costly and must be slowing us down.
2. It mostly works which then leads us to believe that our "shared instance" approach is OK. This is flawed, see bug below).
3. It creates dependencies from the core pattern and expression packages to the security package in P2J. Considering that TPRL and the compiled expressions are not intended to be P2J-specific, these is not wanted. This last issue is not a "deal breaker" but it makes the approach less attractive.

Bug I think the core problem with our current design is that we have a single instance of the CompiledExpression and single instances of Scope which are shared for all threads. That is an inherently flawed design because that same compiled expressions can be executing on multiple threads at the same time. When that happens, it is possible for calls to Variable (e.g. Variable.getValue()) to be made from one thread and then the result to be accessed on another thread (because of a thread context switch). I certainly see that the chances are low in most cases, but the chances are not zero. It is possible for this to occur and it may be the cause of some non-determinism we've seen previously. I wonder if some of the parts of our code in which we force single threading are linked to this issue.

I suspect that we need to split the Scope instances out with separate copies per thread and I am sure that the CompiledExpression instances MUST BE per thread. Although we could try to add more syncrhonization around the expression usage to eliminate this race condition, it would add complexity and overhead into a solution that is already not correct. I don't want to go down that path.

Here is what I suggest to fix this, which also has the good effect of eliminating the ContextLocal usage:

• Compiler.process() currently generates the class AND also calls instantiate(). This means that we only create one instance of the class per compile. We need to split that up. instantiate() should not be called from process(), instead it should be called at first access from a new thread.
• During instantiate(), we copy the libraries and variables. This is the moment that we can make a deep copy of the variable to create a dedicated instance of each variable for each thread. We need to find the right place to call instantiate().
• I am assuming that we have already made all libraries thread safe. If we have not done that, we need to do so. The idea is that any library method that is called will handle any synchronization needed to access or modify the shared data of the library. With this in mind, we can shared instances of the library. The problem I see is for data that needs to be thread local (e.g. how we contain/manipulate references to AST nodes in libraries). A solution would be to keep that as instance data and make instances of the libraries too.
• I think that we need for all classes that descend from Scope to be instantiated per-thread. That will ensure that the maps used to track resources per-scope would be managed on a per-thread basis. I suspect some of the ScopeState could be safely shared but I know that the variable instances are not part of that.

If we decide to remove usages of ContextLocal then I think moving all class descended of Scope to be instantiated per-thread is the best option we have. But this would require instances per-thread at least for PatternEngine.rules or the entire PatternEngine and perhaps AstSymbolResolver as well. Also, we wouldn't need current changes like threadSafe flag because expr.Variable will be involved in that 'deep copy' of the main rule-set. One exception will be H2 collection which are shared anyway by their design through H2 database usage.

2. I agree with Constantin that the use of "threadSafe" is confusing. The normal use of the term means that something is safe for multi-thread access. In our usage here, it means "we will implement synchronization to ensure that this reference is only accessed by one thread at a time". The usage is close but can be confusing. Perhaps we rename this "multiThreadAccess" instead?

Just to clarify: since you mentioned that in our usage here, it means "we will implement synchronization to ensure that this reference is only accessed by one thread at a time", did you actually mean renaming it to singleThreadAccess instead of multiThreadAccess?

3. In multiple places, the TRPL code now uses putIfAbsent(). For example, qualifiedOONames.putIfAbsent(name, create("java.util.TreeSet")) in collect_oo.rules. The problem here is that the code now unconditionally executes the code (in this case create("java.util.TreeSet")) which is very inefficient. The older code only executed this if needed. Since TRPL doesn't directly support lambdas, we should take a different approach. I think this is a place where we need a helper class that implements thread safe operations to replace the original code.

I see the problem. I will rework that.

4. Are we treating the H2Map/H2Set/LoggedCollection as threadsafe? I don't see any logic in those classes that suggests they are threadsafe. All edits and access are unprotected.

I added synchronized to each method used in TRPL. LoggedCollection usages are in synchronized methods as well.

5. It is hard to reason about the collection variables (maps, sets) marked as "threadsafe". I think for each of those definitions, we need a comment explicitly added which explains why the contents are safe for multithread access.

I will add some documentation for those variable definitions.

6. In com.goldencode.expr.SymbolResolver.resetVariables(), there should be no PatternEngine and other references to things in the p2j packages. The fact that we need such references during reset suggests we have something elemental that is not correct in our design.

Yes. I already changed this in rev16414.

7. It is not clear to me why access to com.goldencode.expr.SymbolResolver.instCache is no longer protected by locking.

That protection was replaced with read/write locks in expr.Expression.getCompiledInstance().

Added synchronized block back for getValue and setValue because these methods were modified by mistake after the last rebase. The last revision is 16416.

Constantin Asofiei wrote:

Greg Shah wrote:

Bug I think the core problem with our current design is that we have a single instance of the CompiledExpression and single instances of Scope which are shared for all threads.

Greg, I don't think this is correct; the CompiledExpression is stateless - this is how it looks for i.e. i = i + 1 expression:
[...]

The state is kept via expr.Variable, which in 1752a was changed to allow ContextLocal support. So I don't see how another thread can end up changing the value before the assignment completes.

I disagree. This pattern is not safe. The bytecode for this statement is not atomic, there is no critical section here to avoid a task switch. If there is a task switch between the time the this.getVar(0) is called and when the this.setVar() is invoked, then the result is data corruption.

But I agree that ContextLocal is too aggressive. I chose this approach initially if we need any context-state for runtime conversion, from the TRPL conversion engine like the 'pattern' classes, SchemaDictionary, etc. But from expr package, what we keep there it matters only to the thread - no other thread can access it. So I wonder if from inside expr package, we can maybe switch to ThreadLocal

Maybe, but I think it could still have problems with the libraries. My suggested approach eliminates any need for even thread-local usage.

Also, for runtime conversion, I would like to avoid creating 100k's of CompiledExpression instances, which are short-lived; but this I think needs to be considered with the cost of ThreadLocal access...

This is something to consider.

2. I agree with Constantin that the use of "threadSafe" is confusing. The normal use of the term means that something is safe for multi-thread access. In our usage here, it means "we will implement synchronization to ensure that this reference is only accessed by one thread at a time". The usage is close but can be confusing. Perhaps we rename this "multiThreadAccess" instead?

Just to clarify: since you mentioned that in our usage here, it means "we will implement synchronization to ensure that this reference is only accessed by one thread at a time", did you actually mean renaming it to singleThreadAccess instead of multiThreadAccess?

The idea was to make it clear that there would be multi-threaded access to this same instance. If we say singleThreadAccess, then that has the same implication as threadSafe which is: "this variable is onluuy ever accessed from a single thread so don't worry about synchronization or other locking. That is exactly the opposite impression from what we want to provide.

Some extra notes about the latest ideas:

Greg Shah wrote:

Here is what I suggest to fix this, which also has the good effect of eliminating the ContextLocal usage:

• Compiler.process() currently generates the class AND also calls instantiate(). This means that we only create one instance of the class per compile. We need to split that up. instantiate() should not be called from process(), instead it should be called at first access from a new thread.
• During instantiate(), we copy the libraries and variables. This is the moment that we can make a deep copy of the variable to create a dedicated instance of each variable for each thread. We need to find the right place to call instantiate().
• I am assuming that we have already made all libraries thread safe. If we have not done that, we need to do so. The idea is that any library method that is called will handle any synchronization needed to access or modify the shared data of the library. With this in mind, we can shared instances of the library. The problem I see is for data that needs to be thread local (e.g. how we contain/manipulate references to AST nodes in libraries). A solution would be to keep that as instance data and make instances of the libraries too.
• I think that we need for all classes that descend from Scope to be instantiated per-thread. That will ensure that the maps used to track resources per-scope would be managed on a per-thread basis. I suspect some of the ScopeState could be safely shared but I know that the variable instances are not part of that.

So, compiled needs to be ThreadLocal or store its copies in a map by the current thread-id or something. This way, we create dedicated instances of each thread. But with this in mind, why do we need for all classes that descend from Scope to be instantiated per-thread if they use compiled expressions that would already be instantiated per-thread? Based on this idea were made libraries thread safe in the latest version of 1752a. These use variables that are ContextLocal.

We are trying to avoid ThreadLocal and ContextLocal because they are expensive. Compiled expressions have access to a reference to the Scope in which they reside. By making each Scope an instance that is thread specific (but not ThreadLocal), when the instances of compiled expressions access their reference to Scope, it will already be linked to that thread and we won't have to do any costly map lookups to access it.

Greg Shah wrote:

We are trying to avoid ThreadLocal and ContextLocal because they are expensive. Compiled expressions have access to a reference to the Scope in which they reside. By making each Scope an instance that is thread specific (but not ThreadLocal), when the instances of compiled expressions access their reference to Scope, it will already be linked to that thread and we won't have to do any costly map lookups to access it.

I see that the Scope is accessed through resolver.getCurrentScope() in clone method. So, will RuleContainer, Rule, Variable and Expression be thread-specific?

Yes, that is the idea. Create the instances for each thread and allow them to reference each other directly.

Greg Shah wrote:

Yes, that is the idea. Create the instances for each thread and allow them to reference each other directly.

Including PatternEngine?

Why would PatternEngine need to have multiple instances in a multi-threaded run?

Greg Shah wrote:

Why would PatternEngine need to have multiple instances in a multi-threaded run?

Since it extends RuleContainer, implicitly Scope, I thought it will be used as a start point for the deep copy mechanism.

I think the PE itself should not inherit from Scope, but if needed we can create a top-level class that is the "parent" RuleContainer and is thread-specific.

OK. In such cases, would be a good practice to create a new branch 1752b, or should we keep the work in 1752a?

Octavian Adrian Gavril wrote:

OK. In such cases, would be a good practice to create a new branch 1752b, or should we keep the work in 1752a?

Let's evolve it in 1752a.

Here is a list with classes used as variable types in TRPL:

variable typesvariable types

type="com.goldencode.ast.Aast" 
type="com.goldencode.ast.AnnotatedAst" 
type="com.goldencode.ast.ShadowNode" 
type="com.goldencode.ast.TreeLocal" 
type="com.goldencode.p2j.convert.AttributeMethodDescriptor" 
type="com.goldencode.p2j.convert.BufferList" 
type="com.goldencode.p2j.convert.BufferScopeTracker" 
type="com.goldencode.p2j.convert.NameMappingWorker$MethodInfo" 
type="com.goldencode.p2j.convert.NameMappingWorker$ProgramInfo" 
type="com.goldencode.p2j.pattern.SoapOperation" 
type="com.goldencode.p2j.persist.Database" 
type="com.goldencode.p2j.persist.dialect.Dialect" 
type="com.goldencode.p2j.persist.ORMHandler" 
type="com.goldencode.p2j.persist.orm.Session" 
type="com.goldencode.p2j.persist.P2JIndex" 
type="com.goldencode.p2j.persist.P2JIndexComponent" 
type="com.goldencode.p2j.preproc.PreprocessorHints" 
type="com.goldencode.p2j.report.ReportDefinition" 
type="com.goldencode.p2j.report.server.CustomColumn" 
type="com.goldencode.p2j.report.server.DetailRow" 
type="com.goldencode.p2j.schema.DataModelAst" 
type="com.goldencode.p2j.schema.EntityName" 
type="com.goldencode.p2j.schema.ImportWorker$SqlRecordLoader" 
type="com.goldencode.p2j.schema.PropertyMapper" 
type="com.goldencode.p2j.schema.SchemaConfig" 
type="com.goldencode.p2j.schema.SchemaDictionary" 
type="com.goldencode.p2j.schema.TableHints" 
type="com.goldencode.p2j.schema.TableRelation" 
type="com.goldencode.p2j.uast.AstKey" 
type="com.goldencode.p2j.uast.CallGraphWorker$CallSiteHint" 
type="com.goldencode.p2j.uast.ClassDefinition" 
type="com.goldencode.p2j.uast.FrameAstKey" 
type="com.goldencode.p2j.uast.JavaAst" 
type="com.goldencode.p2j.uast.ProgressAst" 
type="com.goldencode.p2j.uast.ProgressLexer" 
type="com.goldencode.p2j.uast.SymbolResolver" 
type="com.goldencode.p2j.ui.client.gui.MnemonicInfoGuiImpl" 
type="com.goldencode.p2j.util.BaseDataType" 
type="com.goldencode.p2j.util.character" 
type="com.goldencode.p2j.util.date" 
type="com.goldencode.p2j.util.datetime" 
type="com.goldencode.p2j.util.datetimetz" 
type="com.goldencode.p2j.util.decimal" 
type="com.goldencode.p2j.util.LegacyService" 
type="com.goldencode.p2j.util.LegacyServiceParameter" 
type="com.goldencode.p2j.util.LegacyWebPath" 
type="com.goldencode.p2j.util.ScopedDictionary" 
type="com.goldencode.p2j.util.ScopedSymbolDictionary" 
type="com.goldencode.p2j.xml.XmlAst" 
type="com.goldencode.util.CaseInsensitiveHashMap" 
type="com.goldencode.util.CaseInsensitiveHashSet" 

To ensure full support for copying all expr.Variable instances, all of these classes must implement the Cloneable interface.

Octavian Adrian Gavril wrote:

To ensure full support for copying all expr.Variable instances, all of these classes must implement the Cloneable interface.

The requirement is only for variables which are 'global' and their access needs to be thread-safe. So, can you cut down the list to those types, defined in the .xml pipeline directly?

Constantin Asofiei wrote:

Octavian Adrian Gavril wrote:

To ensure full support for copying all expr.Variable instances, all of these classes must implement the Cloneable interface.

The requirement is only for variables which are 'global' and their access needs to be thread-safe. So, can you cut down the list to those types, defined in the .xml pipeline directly?

Since making RuleContainer thread-specific requires us to clone every Rule, Expression, and Variable, I don't think this approach is necessary for global variables only.

In the new implementation, should we compile expressions just after all rules are loaded? Without calling instantiate. Then, in Expression.getCompiledInstance(), we call instantiate but with no locking, as the variables will be copied to create thread-specific instances, right?

We don't want to compile anything that isn't actually executed. So I think we still want to be lazy about that.

Greg Shah wrote:

We don't want to compile anything that isn't actually executed. So I think we still want to be lazy about that.

To clarify: the class generation should be a one-time global operation, whereas the instantiate logic must trigger lazily upon the first access by each individual thread. Is that correct?

Octavian Adrian Gavril wrote:

Greg Shah wrote:

We don't want to compile anything that isn't actually executed. So I think we still want to be lazy about that.

To clarify: the class generation should be a one-time global operation, whereas the instantiate logic must trigger lazily upon the first access by each individual thread. Is that correct?

Yes. I just want to also ensure that the class generation (one-time global op) is only done lazily (when actually accessed for the first time).

1. These are the last classes that need clone() implementation: variable typesvariable types

   type="com.goldencode.p2j.uast.ClassDefinition" 
   type="com.goldencode.p2j.uast.ProgressLexer" 
   type="com.goldencode.p2j.ui.client.gui.MnemonicInfoGuiImpl" 
   type="com.goldencode.p2j.util.character" 
   type="com.goldencode.p2j.util.date" 
   type="com.goldencode.p2j.util.datetime" 
   type="com.goldencode.p2j.util.datetimetz" 
   type="com.goldencode.p2j.util.decimal" 
   type="com.goldencode.p2j.util.LegacyService" 
   type="com.goldencode.p2j.util.LegacyServiceParameter" 
   type="com.goldencode.p2j.util.LegacyWebPath" 
   type="com.goldencode.p2j.util.ScopedDictionary" 
   type="com.goldencode.p2j.util.ScopedSymbolDictionary" 
   type="com.goldencode.util.CaseInsensitiveHashMap" 
   type="com.goldencode.util.CaseInsensitiveHashSet" 
   

2. The RuleContainer logic from PatternEngine is turned in a composition relationship. This way, a PatternEngine instance has a BaseRuleContainer used to apply the loaded rule-set.
3. The BaseRuleContainer uses a custom deep copy mechanism to ensure that rules, includes, functions, and winners are provided as thread-specific instances rather than shared ones. Here is the step where non global variables are cloned within ScopeState.
4. Implementing thread-specific scopes allows for a clear separation between the generation (process) and the instantiation (instantiate) of a compiled expression. The process is executed only once per expression to return a 'blank' instance. This instance then serves as a blueprint; each thread clones this blueprint and instantiates it with its own unique variable instances. This thread-safety is guaranteed by point 3, as both scopes and variables are now strictly thread-specific.

If we process init-rules OR the variable has multi-threaded access, there is no cloning.

I'm currently working on the way global variables are handled because these are extracted from the same parent, the BaseRuleContainer instance. My initial thought was to share the BaseRuleContainer instead of cloning it, but this can be changed if you consider cloning a better approach. That's why, global variables are cloned specifically during the instantiation of compiled expression, whereas non-global variables are cloned earlier, during the scope cloning process.

The current implementation is not stable so I'm holding off on committing the changes until I resolve the remaining issues.

I think SymbolResolver.instCache is used per thread and threads are not using the same blueprint instance. This happens because the Scope instances are cloned now. Maybe a custom equals method for Scope would help to avoid multiple cached instances over the same expression.

I've committed latest changes in 1752a/16417. These include what is documented in #3211-254, plus a few other fixes.

Next-up, I'm cleaning up the thread-specific scopes by removing redundant ContextLocal usages. I'll also be completing the variable types list that need Cloneable implementation and to add a warning for usages of types that don't implement it yet.

Also, what do you think about #3211-255? Would equals method add too much complexity for searching over the instCache?

I think Constantin was right in #3211-248. The clone method for variable types is required only if the variable ends up with a non-null value (got from initialization or assignation in init-rules). And the variable is not accessed by multiple thread (not shared). I was confused because I was thinking that the clone method is mandatory for Variable class as we copy all scopes instances.

I added some logic to check the type of global variables that are not shared and which don't have non-null value after init-rules. But there were just java.util.Map instances.

So, I added support for copying Collection and Map instances when we clone a Variable. Also, an exception is thrown if the variable type cannot be cloned in any way (isn't Cloneable, primitive, Collection or Map). This way, I had to revert unnecessary changes related to clone method in several classes used as variable types.

The new changes are in 16418.

I removed redundant synchronization/ContextLocal usages. The threadSafe flag is renamed into multiThreadAccess. Based on this flag we decided if the Variable instance should be cloned or not.

These changes are in 1752a/16419.

I continue with documentation and recheck the last review items.

I added properly documentation and did some code restructuring in revision 16420.

Code Review Task Branch 1752a Revisions 16413 through 16421

This is a really good set of updates.

1. Please list the parts of conversion that are forced to run in single threaded mode. I'd like you to retest those in multithread mode. I wonder if the race condition I noted in #3211-231 was the cause. We really don't want to force a single thread limitation if not absolutely needed.

2. I'm concerned about the removal of locking around the XML parsing of ASTs. Shouldn't we be limiting that to only be done in a synchronized way?

3. With the current implementation Compiler.compile() is static and synchronized. This means that only one Expression can ever be compiled at one time. I think this is a potential bottleneck. Shouldn't we design this to allow compilation on every thread simultaneously?

4. It seems like the state accessed by Scope.clone(), ScopeState.clone() (actually, pretty much all of the clone methods recently added) should be protected from multi-threaded access. We are making copies of state that could be modified at the same time as it is being copied.

5. Doesn't multiThreadAccess mean that we must synchronize access to a given Variable value. I would expect Variable.get|setValue() to handle this synchronization (or locking) when multiThreadAccess == true. I see external synchronization in CommonAstSupport.atomicOperation() but that would only protect access from that pathway. It seems like an unsafe design and synchronizing on the Variable instance itself is also not a great practice because it is easily disrupted by later edits if they don't understand that is happening.

6. The direct references to AstKey and FrameAstKey from PE.finish() seem like something that should not be there. These are classes in FWD and not in the TRPL implementation itself. So having direct references here seems wrong.

7. Scope.clone(), ScopeState.clone(), ExceptionAction.clone(), @Variable.threadSpecificValue, CommonAstSupport.createSynchronizedList(), RuleContainer.clone() need javadoc.

Greg Shah wrote:

Code Review Task Branch 1752a Revisions 16413 through 16421

This is a really good set of updates.

1. Please list the parts of conversion that are forced to run in single threaded mode. I'd like you to retest those in multithread mode. I wonder if the race condition I noted in #3211-231 was the cause. We really don't want to force a single thread limitation if not absolutely needed.

• Pre-processing P2O Generation (temp-table schema files) - nondeterministic suffixes for temp-table names;
• Compute class names - nondeterministic file names;
• Detecting Frame Interfaces - nondeterministic frame interface names.

I'm gonna analyze and retest these in multi-threaded mode and come back with the results.

2. I'm concerned about the removal of locking around the XML parsing of ASTs. Shouldn't we be limiting that to only be done in a synchronized way?

Are you referring to ASTs persistence from XmlFilePlugin.saveTree? If so, the only case that required locking is now handled by CommonAstSupport.processClassVariables. Otherwise, each AST should be saved by a single unique thread only.

3. With the current implementation Compiler.compile() is static and synchronized. This means that only one Expression can ever be compiled at one time. I think this is a potential bottleneck. Shouldn't we design this to allow compilation on every thread simultaneously?

Yes. This is also synchronized by using the same SymbolResolver.instCacheLock. Previously, read-write locks were used, but these failed to resolve race conditions in the common compiler instance. I'll analyze a different design for that.

4. It seems like the state accessed by Scope.clone(), ScopeState.clone() (actually, pretty much all of the clone methods recently added) should be protected from multi-threaded access. We are making copies of state that could be modified at the same time as it is being copied.

That protection was guaranteed by BaseRuleContainer.ruleList. I observed significant latency caused by that synchronization, particularly within Code Conversion Annotations - likely due to heavy rule dependencies. Given that the main thread’s rule list remains unmodified after the global rules, I believe we can safely remove the synchronization. The main rule list is used for copy mechanism only.

5. Doesn't multiThreadAccess mean that we must synchronize access to a given Variable value. I would expect Variable.get|setValue() to handle this synchronization (or locking) when multiThreadAccess == true. I see external synchronization in CommonAstSupport.atomicOperation() but that would only protect access from that pathway. It seems like an unsafe design and synchronizing on the Variable instance itself is also not a great practice because it is easily disrupted by later edits if they don't understand that is happening.

6. The direct references to AstKey and FrameAstKey from PE.finish() seem like something that should not be there. These are classes in FWD and not in the TRPL implementation itself. So having direct references here seems wrong.

7. Scope.clone(), ScopeState.clone(), ExceptionAction.clone(), @Variable.threadSpecificValue, CommonAstSupport.createSynchronizedList(), RuleContainer.clone() need javadoc.

Understood.I'll take care of that.

These are the phases that run in single threaded mode:

Don't we also use single threaded mode for legacy proxygen stuff?

The ones you note above are all related to stable naming. I think we should consider a solution there that is more general purpose, but we may want to defer that to another phase. We have other reasons to solve this stability problem, for example: app-by-app conversion.

2. I'm concerned about the removal of locking around the XML parsing of ASTs. Shouldn't we be limiting that to only be done in a synchronized way?

Are you referring to ASTs persistence from XmlFilePlugin.saveTree?

Yes

If so, the only case that required locking is now handled by CommonAstSupport.processClassVariables. Otherwise, each AST should be saved by a single unique thread only.

That surprises me. We have recursive parsing used when OO classes are involved. That can execute the parser or load from an AST depending on whether parsing has finished or not. We need to ensure that we only ever parse/load a given file from one thread at a time. I don't see how we are doing that properly today.

I'm not sure locking in the XML load/save is the right approach but I want to ensure that we have a clean and reliable way to ensure this is safe. I also worry that we need a better concept of dependency processing for the OO classes otherwise it seems like a source of potential deadlocks (one locking is added).

4. It seems like the state accessed by Scope.clone(), ScopeState.clone() (actually, pretty much all of the clone methods recently added) should be protected from multi-threaded access. We are making copies of state that could be modified at the same time as it is being copied.

That protection was guaranteed by BaseRuleContainer.ruleList. I observed significant latency caused by that synchronization, particularly within Code Conversion Annotations - likely due to heavy rule dependencies. Given that the main thread’s rule list remains unmodified after the global rules, I believe we can safely remove the synchronization. The main rule list is used for copy mechanism only.

It is not at all obvious why BaseRuleContainer.ruleList() makes this thread safe. Please explain in more detail.

My worry here is that implementing thread safety in this way relies on future editors understanding this dynamic. One can't easily see that it is safe. So it will be fragile and future edits could easily break adding other usage of clone() from non-safe locations or if edits to state happen in parallel.

On the other hand, if synchronization kills performance, that is also an issue. Let's discuss this further.

Greg Shah wrote:

These are the phases that run in single threaded mode:

Don't we also use single threaded mode for legacy proxygen stuff?

No. That phase doesn't require single threaded execution to have a deterministic output.

If so, the only case that required locking is now handled by CommonAstSupport.processClassVariables. Otherwise, each AST should be saved by a single unique thread only.

That surprises me. We have recursive parsing used when OO classes are involved. That can execute the parser or load from an AST depending on whether parsing has finished or not. We need to ensure that we only ever parse/load a given file from one thread at a time. I don't see how we are doing that properly today.

I'm not sure locking in the XML load/save is the right approach but I want to ensure that we have a clean and reliable way to ensure this is safe. I also worry that we need a better concept of dependency processing for the OO classes otherwise it seems like a source of potential deadlocks (one locking is added).

My previous changes focused on locking within saveTree to handle AST persistence from the TRPL implementation. Are your concerns specifically regarding the parsing phase?

That protection was guaranteed by BaseRuleContainer.ruleList. I observed significant latency caused by that synchronization, particularly within Code Conversion Annotations - likely due to heavy rule dependencies. Given that the main thread’s rule list remains unmodified after the global rules, I believe we can safely remove the synchronization. The main rule list is used for copy mechanism only.

It is not at all obvious why BaseRuleContainer.ruleList() makes this thread safe. Please explain in more detail.

Once the global rules are done, the main rule-set reaches its terminal state. From that point, all operations are strictly read-only; since no further modifications occur, concurrent access is thread-safe.

My worry here is that implementing thread safety in this way relies on future editors understanding this dynamic. One can't easily see that it is safe. So it will be fragile and future edits could easily break adding other usage of clone() from non-safe locations or if edits to state happen in parallel.

I see your point.

On the other hand, if synchronization kills performance, that is also an issue. Let's discuss this further.

Exactly. My primary concern was that the locking overhead would become a significant bottleneck during high-volume execution. Perhaps, I could check if the things are better if we make each clone method synchronized individually for each component.

Greg Shah wrote:

6. The direct references to AstKey and FrameAstKey from PE.finish() seem like something that should not be there. These are classes in FWD and not in the TRPL implementation itself. So having direct references here seems wrong.

Should we delegate cleaning of these caches to CommonAstSupport library? We can call a helper method from post-rules, in .xml files where AstKey and FrameAstKey are used.

If so, the only case that required locking is now handled by CommonAstSupport.processClassVariables. Otherwise, each AST should be saved by a single unique thread only.

That surprises me. We have recursive parsing used when OO classes are involved. That can execute the parser or load from an AST depending on whether parsing has finished or not. We need to ensure that we only ever parse/load a given file from one thread at a time. I don't see how we are doing that properly today.

I'm not sure locking in the XML load/save is the right approach but I want to ensure that we have a clean and reliable way to ensure this is safe. I also worry that we need a better concept of dependency processing for the OO classes otherwise it seems like a source of potential deadlocks (one locking is added).

My previous changes focused on locking within saveTree to handle AST persistence from the TRPL implementation. Are your concerns specifically regarding the parsing phase?

No, not exclusively. I do think we have a potential threading issue during parsing. But we might also have a problem with reading and writing the XML AST files. I think we need a clean way to manage this that is thread safe.

That protection was guaranteed by BaseRuleContainer.ruleList. I observed significant latency caused by that synchronization, particularly within Code Conversion Annotations - likely due to heavy rule dependencies. Given that the main thread’s rule list remains unmodified after the global rules, I believe we can safely remove the synchronization. The main rule list is used for copy mechanism only.

It is not at all obvious why BaseRuleContainer.ruleList() makes this thread safe. Please explain in more detail.

Once the global rules are done, the main rule-set reaches its terminal state. From that point, all operations are strictly read-only; since no further modifications occur, concurrent access is thread-safe.

My concern is with the internal state of each variable or resource that could be cloned. Any code that has a reference to a variable instance, could change the internal state at any time. I don't see how ruleList() eliminates that.

My worry here is that implementing thread safety in this way relies on future editors understanding this dynamic. One can't easily see that it is safe. So it will be fragile and future edits could easily break adding other usage of clone() from non-safe locations or if edits to state happen in parallel.

I see your point.

On the other hand, if synchronization kills performance, that is also an issue. Let's discuss this further.

Exactly. My primary concern was that the locking overhead would become a significant bottleneck during high-volume execution. Perhaps, I could check if the things are better if we make each clone method synchronized individually for each component.

If the previous locking was being done at the caller, then that indeed would perform poorly. The locking shoudl certainly be done within each class where the clone() is implemented and the locking should be instance-specific (not class-wide unless the resource is static).

About Expression.getCompiledExpression. Is it efficient to allow redundant compilation of identical expressions across multiple threads? Would it be more performant to implement a synchronization strategy where the first thread compiles the expression (process), while subsequent threads retrieve a cached instance, clone it, and then instantiate the clone?

Octavian Adrian Gavril wrote:

About Expression.getCompiledExpression. Is it efficient to allow redundant compilation of identical expressions across multiple threads?

No, I wouldn't expect so.

Would it be more performant to implement a synchronization strategy where the first thread compiles the expression (process), while subsequent threads retrieve a cached instance, clone it, and then instantiate the clone?

Yes, this seems to be a better way. One should obtain a lock based on the expression text.

Is there a way to see how a compiled expression class looks like?

I have some errors like java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 for such expression: ref = this.getImmediateChild(prog.kw_proc, null).getImmediateChild(prog.symbol, null). There is only one variable: ref. The compiled expression seems fine, but its execution wants more variables. Looks like another compiled expression is executed, but I can't see why.

3. With the current implementation Compiler.compile() is static and synchronized. This means that only one Expression can ever be compiled at one time. I think this is a potential bottleneck. Shouldn't we design this to allow compilation on every thread simultaneously?

This is fixed in rev 16423. I've added a lock cache based on expression text and fixed some issues from Compiler that were causing troubles with generated class names and class loading. Concurrent compilation is now supported.

I had to implement equals and hashCode for Scope, ScopeState and expr/Variable to support value-based equality checks required by the expression cache lookup. This way, we ensure that threads share the same compiled expression, then clone it.

Implementing equals and hashCode for Scope broke how includes are processed in RuleContainer because a Set is used for that. I had to move includes from Set to List because two instances of Scope with no variables or libraries are considered equal.

The improvement is noticeable. I'm curious about the impact of these changes on runtime conversion.

The improvement is noticeable. I'm curious about the impact of these changes on runtime conversion.

YES! Please measure the difference in both ConversionDriver and in runtime conversions.

I've committed new changes in 16424. These include initial support for multi-threaded parsing, but the solution is not fully functional. I'm currently encountering nondeterministic execution for the same builtin classes ASTs. The main issues is about some missing annotations like:

--- /home/aog/gcd/oo_testcase_project_orig/cvt/skeleton/oo4gl/OpenEdge/Core/Assertion/AssertObject.cls.ast    2026-03-13 09:48:07.607622955 +0200
+++ /home/aog/gcd/oo_testcase_project/cvt/skeleton/oo4gl/OpenEdge/Core/Assertion/AssertObject.cls.ast    2026-03-16 13:02:38.889632981 +0200
@@ -1,26 +1,22 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
 <!--Persisted AST--><ast-root ast_class="com.goldencode.p2j.uast.ProgressAst">
   <ast col="0" id="283467841537" line="0" text="block" type="BLOCK">
     <annotation datatype="java.lang.Boolean" key="builtin-cls" value="true"/>
     <annotation datatype="java.lang.Boolean" key="dotnet-cls" value="false"/>
     <ast col="0" id="283467841538" line="0" text="statement" type="STATEMENT">
       <ast col="1" id="283467841539" line="1" text="using" type="KW_USING">
         <annotation datatype="java.lang.String" key="qualified" value="openedge.core.collections.icollection"/>
-        <annotation datatype="java.lang.String" key="source-file" value="./skeleton/oo4gl/OpenEdge/Core/Collections/ICollection.cls"/>
+        <annotation datatype="java.lang.String" key="source-file" value="./skeleton/oo4gl/OpenEdge/Core/Assertion/AssertObject.cls"/>
         <annotation datatype="java.lang.Boolean" key="builtin-cls" value="true"/>
         <annotation datatype="java.lang.Boolean" key="dotnet-cls" value="false"/>
-        <annotation datatype="java.lang.Boolean" key="indirect-dotnet" value="false"/>
-        <annotation datatype="java.lang.Boolean" key="is-class" value="false"/>
-        <annotation datatype="java.lang.Boolean" key="is-interface" value="true"/>
-        <annotation datatype="java.lang.Boolean" key="is-enum" value="false"/>

These should be populated in SymbolResolver.annotateClassRef. But I think it's a persisting/loading race between threads.

In 16424 I added synchronization to XmlFilePlugin.loadTree, and AST persistence is already protected by AstGenerator.processFile. Some fields in SymbolResolver.WorkArea needed to be shared across threads while others did not.

Class loading uses a separate lock from AstGenerator.processFile, and I had to bring pre-scan checking and hierarchy parsing outside of that lock to avoid circular wait. Despite these changes and several additional locking experiments in other areas, the issue still persists.

Greg, Constantin, can you advise me, please?

These should be populated in SymbolResolver.annotateClassRef. But I think it's a persisting/loading race between threads.

This AST corruption problem means that there is state somehow shared across threads that should be thread-specific. We have to isolate AST creation and access to be strictly thread-specific.

Class loading uses a separate lock from AstGenerator.processFile, and I had to bring pre-scan checking and hierarchy parsing outside of that lock to avoid circular wait. Despite these changes and several additional locking experiments in other areas, the issue still persists.

Hmmm. We've had problems with this before. I think much (but not all) of the problem relates to the fact that were are using skeletons to resolve things related to the built-in classes. This is parsing and processing that OE doesn't do and eliminating this would probably solve almost all of the problem. This was intended to be solved by a combination of #9638 (using annotations instead of ASTs) and #9488 (including the full range of built-in classes into FWD).

There is also the potential problem of circular dependencies. That only would come into play in very specific cases and it probably is not a the problem here. But we will indeed need to explore this and fix it at some point (I suspect).

For now, I think we should drop back to single-threaded mode for parsing. Please create a separate task for implementing multi-threaded parsing. We will defer the work until then. My idea is: let's get 1752a stabiloized, tested and merged. Then we complete #9638 and #94888. Then we do the multi-threaded parsing and other improvements to multi-threading.

Constantin?

Greg Shah wrote:

For now, I think we should drop back to single-threaded mode for parsing. Please create a separate task for implementing multi-threaded parsing. We will defer the work until then. My idea is: let's get 1752a stabiloized, tested and merged. Then we complete #9638 and #94888. Then we do the multi-threaded parsing and other improvements to multi-threading.

OK. But we still need to take care of this issue on 1752a, right?

The ones you note above are all related to stable naming. I think we should consider a solution there that is more general purpose, but we may want to defer that to another phase. We have other reasons to solve this stability problem, for example: app-by-app conversion.

Yes, the stable naming issue has to be handled here.

Greg Shah wrote:

Yes, the stable naming issue has to be handled here.

Sounds good. I’m rebasing 1752a next and continuing my investigation into refactoring of deterministic naming.

Rebased branch 1752a with trunk/16468. The new revision is 16538. Added post-rebase fixes in 16539.

Reverted multi-threaded parsing changes and the new revision is 16540. After retesting the new revision against the Hotel_GUI and ChUI regression testing apps, I identified several rule breakages caused by compatibility mismatches with the latest trunk updates. So, I fixed those by replacing file with artifact.getRelativePath().

I also addressed differences regarding SharedVariableManager.addTempTable and TemporaryBuffer.define, specifically a missing no-undo flag on the 1752a side. Finally, I've noticed differences from #10935 into post_parse_fixups and integrated them for multi-threaded processing. Most issues are resolved, though one persistent difference remains under investigation.

-   Tt_1_1.Buf tt = TemporaryBuffer.define(tt_1_1.Buf.class, "tt", "tt", false, false);
+   Tt_1_1.Buf tt = TemporaryBuffer.define(tt_1_1.Buf.class, "tt", "tt", false);

I’m going to prioritize the stable naming fixes first, then I’ll investigate the fix for this remaining difference.

The name registration phase runs before annotations/annotations.xml, which is responsible for setting up the conditions/processing that frame_generator_pre relies on to filter which frames are processed. Without replicating those conditions in the new phase, frameInterfaces ends up populated with far more frame entries than expected.

This is the main condition that I need to replicate in order to calculate stable names properly:

         <rule>type == prog.frame_alloc and
            #(boolean) copy.getAnnotation("shared")
            <action>unFormedFrames.put(copy.id, copy)</action>
         </rule>

I am looking at the following situation in our rules:

         <rule>type == prog.kw_form or
            (type == prog.define_frame and
            copy.getImmediateChild(prog.form_item, null) != null)
            <!-- basic name: extract frame name -->
            <action>frName = ""</action> <!-- default -->
            <action>ref = copy.getImmediateChild(prog.frame_phrase, null)</action>
            <rule>ref != null
               <action>ref = ref.getImmediateChild(prog.kw_frame, null)</action>
               <rule>ref != null
                  <action>ref = ref.getImmediateChild(prog.wid_frame, null)</action>
                  <rule>ref != null
                     <action>frName = ref.text</action>
                  </rule>
               </rule>
            </rule>

            <!-- detect if frame is SHARED (NEW or dependent), we skip independent frames for this
                 analysis -->
            <action>ref = unFormedFrames.get(#(java.lang.Long) copy.getAnnotation("frame-id"))</action>
            <rule>ref != null and #(boolean) ref.getAnnotation("shared")
               <rule>not ref.isAnnotation("fr_interface")
                  <action>formKey = create("com.goldencode.p2j.uast.FrameAstKey", copy, frName)</action>

A FORM statement that doesn't have a frame_phrase results in frName remaining at its default value. What would be the final name in this scenario? Also, how would such a case be represented when dealing with a shared frame?

I need to avoid using the frame-id annotation, as it is not yet attached during the stable naming phase. So, I must key unFormedFrames by the frame name. But, in these specific cases, frName may result in an empty string.

Ovidiu, do you know how a FORM should be processed when the frame name defaults to an empty string ""? What's the expected behavior in that case?

Some UI language statements which can define a frame can be specified without a frame name. Actually, all of the frame defining UI statements can be specified without a frame name, except for DEFINE FRAME. When you do this, it implicitly refers to the "unnamed frame" (a.k.a. the default frame). There can be one unnamed frame for the external procedure and each internal procedure can also have its own unnamed frame. These work just like any other frame but they have no name and the "linkage" is implicit instead of explicit.

Examples include:

FORM var1 var2.
FORM var3 WITH 5 DOWN.
DISPLAY field4 field5.
UPDATE var7 WITH TITLE "Unnamed Frame".

Notice that it doesn't just happen with FORM and it doesn't mean that there is no frame phrase (WITH). The key is that there is no explicitly named frame.

Greg Shah wrote:

Some UI language statements which can define a frame can be specified without a frame name. Actually, all of the frame defining UI statements can be specified without a frame name, except for DEFINE FRAME. When you do this, it implicitly refers to the "unnamed frame" (a.k.a. the default frame). There can be one unnamed frame for the external procedure and each internal procedure can also have its own unnamed frame. These work just like any other frame but they have no name and the "linkage" is implicit instead of explicit.

Examples include:

[...]

Notice that it doesn't just happen with FORM and it doesn't mean that there is no frame phrase (WITH). The key is that there is no explicitly named frame.

OK, I understand.

But, if we have a DEFINE_FRAME having a WITH frame clause (that doesn't mention the frame name because it's a frame definition), in that case the frName is still "". However, we have the frame name from the SYMBOL child. So, in this case I think the default should be overwrite by SYMBOL text. Am I right?

Another thing. !unFormedFrames.isEmpty check from frame_generator_pre.xml>rule-set>post-rules is supposed to create frame names for declared but unused frames, right? But there is no chance to increase their suffix more than once. So, they don't go through processForm in any other file.

But, if we have a DEFINE_FRAME having a WITH frame clause (that doesn't mention the frame name because it's a frame definition), in that case the frName is still "". However, we have the frame name from the SYMBOL child. So, in this case I think the default should be overwrite by SYMBOL text. Am I right?

Yes, that is exactly right.

Octavian Adrian Gavril wrote:

Another thing. !unFormedFrames.isEmpty check from frame_generator_pre.xml>rule-set>post-rules is supposed to create frame names for declared but unused frames, right? But there is no chance to increase their suffix more than once. So, they don't go through processForm in any other file.

This sounds like a bug but I don't have time to look at it. See if Hynek can give some advice.

Moving the name computation to an earlier phase causes EMPTY_FORM_AST to be created twice: once in stable naming phase and once in frame_generator_pre, resulting in FrameAstKey deserialization failures (as the first EMPTY_FORM_AST no longer exists).

Octavian Adrian Gavril wrote:

Another thing. !unFormedFrames.isEmpty check from frame_generator_pre.xml>rule-set>post-rules is supposed to create frame names for declared but unused frames, right? But there is no chance to increase their suffix more than once. So, they don't go through processForm in any other file.

Hynek, my concern is whether we can simply name remaining frames from unFormedFrames as Frame[frName]_1, given that their suffix should never need to increment. Or is that assumption incorrect?

I've committed new changes about stable naming generation in rev16541.
I removed all single-threaded switching from ConversionDriver and now we have a dedicated phase called Stable naming. This is executed in multi-threaded mode as well, right before Pre-processing P2O Generation (temp-table schema files). The timing of this phase is driven by the dependency on .schema artifacts.

• filenames that are shared (excluding extension)
  • e.g. a/b/start.p and a/b/start.w
  • these were previously handled by Compute class names phase (that is now removed).
• temp-table interface + temp-table implementation names
  • these were previously handled before by Pre-processing P2O Generation (temp-table schema files) phase (that logic is now moved to stable_naming.xml).
• frame interface names
  • these were previously handled before by Detecting Frame Interfaces phase (that logic is now moved to stable_naming.xml).

All sections follow the same strategy. Walk through ASTs in a multi-threaded environment and store required information like formKey, tmpTabKey, base names and artifact paths. Then, in global post-rules make the suffixed name assignments.

Continue testing the solution and resolve any conversion differences.

I've added some fixes related to conversion differences in rev16542.

Constantin, I’ve been looking into the changes for #10935 and wanted to clarify the intended scope. Should we be analyzing temp-tables across all files, or just the current ones?

In post_parse_fixups, we use the global map tableTempIdx to store annotations. Because this map is global, a collision occurs if two different temp-tables share the same tempIdx. Specifically, we end up checking the NO-UNDO or LIKE nodes for only one of them, but we incorrectly apply those results to both.

I ran into this during ChUI regression testing. I have a shared temp-table a defined LIKE b (where b is a database table), but its AST shares a tempIdx with a completely different NO-UNDO temp-table c.

Do you think we should make the tableTempIdx map local to the file, or is there a specific reason it needs to remain global?

I've noticed that all testcases from #10935 are using a single procedure in order to reproduce the issue. About classes, I've seen that the problem is fixed in SchemaDictionary. So, the changes from post_parse_fixups are not related to testcases based on multiple .cls files.

Octavian Adrian Gavril wrote:

Do you think we should make the tableTempIdx map local to the file, or is there a specific reason it needs to remain global?

Thanks for noticing this. Yes, make it local to the file, so is not per-pipeline.

I've committed new changes into 1752a/16543. These include the fix mentioned in #3211-293 + I had an issue with conflicting paths check from stable_naming. That one is responsible for checking if the current artifact has a potential conflict related to its final class name. The previous implementation used artifact.getRelativePath() which returns the name of the source file (*.[p|w|cls]) if we need to handle the .ast file. So, I refactored to processing of conflicting path in order to support the current implementation of artifact.getRelativePath().

I also wanted to fix the differences documented in #3211-102, as they reappear after we moved frame_generator_pre.xml back to multi-threaded mode. I moved the logic related to master frames from frame_generator_pre.xml to stable_naming.xml. Although my latest changes were technically stable, they generated too much noise across multiple files regarding setExpr methods. In contrast, the approach from #3211-102 delivers stable results while only impacting one file. Furthermore, these differences appear consistent, as I haven't observed any output for that file other than what was reported in #3211-102 (there is likely a logical explanation for this). To keep the codebase clean and the logic predictable, I’ve decided to preserve the original master frame logic in frame_generator_pre.xml

I've tested the latest revision with Hotel_GUI and ChUI regression testing apps and there are no unexpected changes. Continue testing with two more apps and if there will be no issues, then we can proceed reviewing the code.

Rebased branch 1752a with trunk/16495. The new revision is 16570.

I am currently testing the conversion of a large OO application, but the process consistently blocks at the exact same point for the same files. Despite the "frozen" behavior, profiling shows that the threads are not technically deadlocked. However, thread dumps show them all busy within loadTree, specifically at the readAst, createAst, or parse methods.

I've already tried to scale the memory, but the issue persists. I also reverted some changes that replaced HashSet for LinkedHashSet to exclude memory overhead, yet the freeze still occurs. I’m going to run conversion on other large-scale apps to determine if this is a global logic issue or specific to this project's structure.

The issue persists with a large GUI app as well. The job queue size isn't dropping, but the threads are staying busy by constantly cycle-processing other tasks. This confirms that we have a problem in the class dependency mechanism, where jobs are being perpetually re-queued instead of completed.

Committed the fix for class dependency mechanism in rev16571. The keys used for fileStatusMap by inherited class/base class were broken. These were not the same due to mismatched output of source-file annotation and artifact.getRelativePath().

The conversion for a large OO app completed successfully. I'm going to analyze the differences.

• dialect conflicts: several class names were updated from Authorization to Authorization_, with corresponding updates made to their usages.
• static imports: the missing import static com.goldencode.p2j.util.ProcedureManager.*; in certain files is expected, as this was addressed in a previous fix.
• stable naming: differences occur in two areas:
  • temp-table name suffixes (and their associated filenames/references) have changed.
  • similar naming shifts are present in files with identical names, as documented in #3211-115.

Continue testing a large GUI app.

temp-table name suffixes (and their associated filenames/references) have changed.

Is this stable (just a one time shift seen when 1752a is applied)?

similar naming shifts are present in files with identical names, as documented in #3211-115.

You mean identical base names? Also, would this happen for files with case differences (on systems like Linux which are case-sensitive)?

These scenarios also need to be deterministic. Can't we just sort in a stable way and then always apply the same _<num> based on that order?

Greg Shah wrote:

temp-table name suffixes (and their associated filenames/references) have changed.

Is this stable (just a one time shift seen when 1752a is applied)?

Yes.

similar naming shifts are present in files with identical names, as documented in #3211-115.

You mean identical base names? Also, would this happen for files with case differences (on systems like Linux which are case-sensitive)?

This depends on NameConvertWorker.convert method.

These scenarios also need to be deterministic. Can't we just sort in a stable way and then always apply the same _<num> based on that order?

The current scenario is that we have a/b/start.p and a/b/start.w, the converted class name for both is Start. But this is impossible, so we will have Start.java and Start_1.java. This processing is stable and deterministic (fixed by stable_naming.xml), but it doesn't match the baseline output.

Octavian Adrian Gavril wrote:

Greg Shah wrote:

temp-table name suffixes (and their associated filenames/references) have changed.

Is this stable (just a one time shift seen when 1752a is applied)?

Yes.

similar naming shifts are present in files with identical names, as documented in #3211-115.

You mean identical base names? Also, would this happen for files with case differences (on systems like Linux which are case-sensitive)?

This depends on NameConvertWorker.convert method.

These scenarios also need to be deterministic. Can't we just sort in a stable way and then always apply the same _<num> based on that order?

The current scenario is that we have a/b/start.p and a/b/start.w, the converted class name for both is Start. But this is impossible, so we will have Start.java and Start_1.java. This processing is stable and deterministic (fixed by stable_naming.xml), but it doesn't match the baseline output.

OK, that is fine. I'm surprised there is a difference for this second case. We already sort the source file list lexicographically. I would have expected start.w to have the _1 even in the old implementation.

Greg Shah wrote:

OK, that is fine. I'm surprised there is a difference for this second case. We already sort the source file list lexicographically. I would have expected start.w to have the _1 even in the old implementation.

I see. I will double-check if the the new implementation sort the source file list the same as the old one to avoid such unnecessary differences.

The conversion for a large GUI application failed, and I am currently investigating the root cause. Additionally, the baseline used was generated from a trunk version that lacks the fix for #3211-293. As a result, there are many differences involving the NO-UNDO flag.

Octavian Adrian Gavril wrote:

Greg Shah wrote:

OK, that is fine. I'm surprised there is a difference for this second case. We already sort the source file list lexicographically. I would have expected start.w to have the _1 even in the old implementation.

I see. I will double-check if the the new implementation sort the source file list the same as the old one to avoid such unnecessary differences.

I fixed this but now I get case-sensitive on false for OO app conversion with 1752a. I've noticed that this would be expected as opsys is set on WIN32. But still, the conflicting file names are case sensitive with trunk, even if the configuration is the same.

Octavian Adrian Gavril wrote:

I fixed this but now I get case-sensitive on false for OO app conversion with 1752a. I've noticed that this would be expected as opsys is set on WIN32. But still, the conflicting file names are case sensitive with trunk, even if the configuration is the same.

I think I found the root cause. In trunk we use other variable for the class name, not the one over which we apply toLowerCase. I'm retesting the changes with OO app.

Octavian Adrian Gavril wrote:

Greg Shah wrote:

OK, that is fine. I'm surprised there is a difference for this second case. We already sort the source file list lexicographically. I would have expected start.w to have the _1 even in the old implementation.

I see. I will double-check if the the new implementation sort the source file list the same as the old one to avoid such unnecessary differences.

I committed the fix for this in rev16573.

What was the issue?

Greg Shah wrote:

What was the issue?

The filter designed to identify conflicting files (such as a/b/start.p and a/b/start.w) was failing due to a path mismatch. While conflictingArtifacts stored paths in the format ./cvt/.../*.ast, the system was attempting to query them using the ./abl/.../*.p format.

Octavian Adrian Gavril wrote:

The conversion for a large GUI application failed, and I am currently investigating the root cause. Additionally, the baseline used was generated from a trunk version that lacks the fix for #3211-293. As a result, there are many differences involving the NO-UNDO flag.

The root cause was a race condition in the non-synchronized BufferScopeWorker.getSuperTable method. Because this method retrieves tables from a class's super-classes, concurrent access by multiple threads processing the same class definition leads to data conflicts. I retested the conversion of a large GUI app with the synchronized method and it completed successfully. Run-to-run stability should be check for all project in the future to ensure the conversion results are repeatable.

• As expected, temp-table name suffixes (and their associated filenames/references) have changed, mirroring the behavior of the OO app. This is a direct result of the sorting logic implemented in stable_naming.
• There are some differences in the generated MetaUser class that represents the table from mscdata. The column name of some properties are different: 1752a/userid vs trunk/_Userid. This bug looks familiar to me, but I didn't find any note related to this. I'm investigating this as well.

I found a regression from rev16573 where getPrimaryIndex() failed due to an incomplete P2OLookup instance. The issue was caused by a change in execution order: now that name computing for conflicting file is working, p2o.isDMOInterface runs in stable_naming as well, before P2O Post-processing (database schema files). This resulted in a lookup instance with an empty indexMap (or an incomplete map).

I fixed this by clearing the P2OLookup cache in the stable_naming -> post-rules, forcing a complete reload of instances in later conversion phases. I'm retesting the conversion of OO app.

Octavian Adrian Gavril wrote:

The root cause was a race condition in the non-synchronized BufferScopeWorker.getSuperTable method. Because this method retrieves tables from a class's super-classes, concurrent access by multiple threads processing the same class definition leads to data conflicts. I retested the conversion of a large GUI app with the synchronized method and it completed successfully.

I found a regression from rev16573 where getPrimaryIndex() failed due to an incomplete P2OLookup instance. The issue was caused by a change in execution order: now that name computing for conflicting file is working, p2o.isDMOInterface runs in stable_naming as well, before P2O Post-processing (database schema files). This resulted in a lookup instance with an empty indexMap (or an incomplete map).

I fixed this by clearing the P2OLookup cache in the stable_naming -> post-rules, forcing a complete reload of instances in later conversion phases. I'm retesting the conversion of OO app.

The OO app conversion completed successfully. I've committed the changes relates with these two items in rev16574.

Code Review Task Branch 1752a Revisions 16563 through 16575

Overall, this is a really good update.

1. I don't understand why stable_naming is run conditionally when there is no schemata but the naming affects OO classes and frames too. This needs to be run unconditionally.

2. Why is stable_naming part of the middle processing? That is supposed to be something specific to database but the naming affects OO classes and frames too. It probably should be part of early annotations.

3. The conflicting artifact processing in PatternEngine seems like it would be better placed in the ArtifactManager as standard logic there.

Greg Shah wrote:

Code Review Task Branch 1752a Revisions 16563 through 16575

Overall, this is a really good update.

Thanks for the review!

1. I don't understand why stable_naming is run conditionally when there is no schemata but the naming affects OO classes and frames too. This needs to be run unconditionally.

2. Why is stable_naming part of the middle processing? That is supposed to be something specific to database but the naming affects OO classes and frames too. It probably should be part of early annotations.

These two points are related because I placed that phase there specifically to ensure the schema files were already generated. An earlier position wouldn't have been helpful for that reason. Perhaps we could call that phase twice: first using only ASTs to handle naming for OO classes and frames, and a second time within the schema block for temp table interfaces and implementations.

3. The conflicting artifact processing in PatternEngine seems like it would be better placed in the ArtifactManager as standard logic there.

I agree. I'm moving that logic to ArtifactManager now.

Octavian Adrian Gavril wrote:

Greg Shah wrote:

3. The conflicting artifact processing in PatternEngine seems like it would be better placed in the ArtifactManager as standard logic there.

I agree. I'm moving that logic to ArtifactManager now.

Done in rev16576.