TRPL

Our existing documentation is here:

• Writing TRPL Rule-Sets - This defines the language at the level of statements, control flow and program structure. All of this is defined in XML format and is read by the ConfigLoader class. Overall these parts of TRPL are implemented in the com.goldencode.p2j.pattern.* package, see the PatternEngine for the main "runner" of a TRPL ruleset.
• Writing TRPL Expressions - This defines the expressions executed (as part of control flow or as a standalone rule/action) to actually do something. We compile these expressions using the com.goldencode.expr.Compiler which uses the expression.g grammar (see the expression() rule) to parse each expression.

Please review the documentation and code in detail. Make sure every TRPL v1 feature is documented somewhere in that documentation so that we have a complete list.

After that, we want to instrument the ConfigLoader and expression Compiler to track the usage of each feature. We can process all of the TRPL rules we have to then see what features are critical and what features can possibly be dropped.

I reviewed ConfigLoader.java along with Writing_TRPL_Rule-Sets and these are some features that I found undocumented:

Continue the same with Compiler and Writing_TRPL_Expressions.

I've reviewed Writing_TRPL_Expressions along with the associated code and it seems fully documented to me. I'm moving on to find a way to track the usage of each TRPL feature.

Greg, where is PatternEngine.main called in order to run the entire TRPL rule-set? I saw some targets in build_db.xml but I don't think this is related as it ran for import.db.pg.

Look for any location that instantiates new PatternEngine(). For example, in TransformDriver.processTrees().

Octavian Adrian Gavril wrote:

I've reviewed Writing_TRPL_Expressions along with the associated code and it seems fully documented to me. I'm moving on to find a way to track the usage of each TRPL feature.

I'm worried that these Expression Features are not documented in detail. We mention that they are available but we provide no comprehensive explanation. Worse: the features are hidden deep in the compiler because they are implemented directly in the generated bytecode. Please take a look at these features (auto-boxing, automatic type conversion, automatic null checking and property notatation).

Greg Shah wrote:

I'm worried that these Expression Features are not documented in detail. We mention that they are available but we provide no comprehensive explanation. Worse: the features are hidden deep in the compiler because they are implemented directly in the generated bytecode. Please take a look at these features (auto-boxing, automatic type conversion, automatic null checking and property notatation).

I've updated the description of the first two features from Expression Features. Should I include more details about the used methods? (e.g. Compiler.convertToRequiredType and how the bytecode is generated using those helper classes)

Greg, please let me know if I should use the same structure for the others or if improvements are needed.

I added some documentation for Property Notation feature. Still looking for the Automatic Null Checking implementation to expand its documentation.

I've updated the description of the first two features from Expression Features.

The changes are really good.

Should I include more details about the used methods? (e.g. Compiler.convertToRequiredType and how the bytecode is generated using those helper classes)

More details are always useful, especially when we are going to rework the language to reimplement these features.

Greg, please let me know if I should use the same structure for the others or if improvements are needed.

Your approach is really good.

Still looking for the Automatic Null Checking implementation to expand its documentation.

Eric: Can you please point us to the proper code?

Eric, is Automatic Null Checking related to code sections similar to this?

         // Create a bogus AST which represents the destination of a jump
         // in the case where the object reference under test is determined
         // to be non-null.
         ExtraAst notNullDest = new ExtraAst();
         notNullDest.setType(NOT_NULL);
         notNullDest.setText("null check dest");
         notNullDest.setExtra(finishUnit);

Question from Octavian:

Should I configure ConfigLoader and Compiler to track each .rules file/.xml file/rule-set/function? Is there something I missed?

We might need that information (eventually) but for now I am more worried about getting a count of how many times each language feature is used. It seems to me that some features are parsed via the ConfigLoader and some parsed by the expression.g. We can count many features by instrumenting these pieces of code and then loading all the rules. There are also features that can only be seen by the compiler because they are implemented implicitly during compilation, so it seems that some instrumentation there could also be needed to get a full count.

The objective is to see if there are features of the language that are not really being used. Dropping those features would make it easier to implement TRPL v2.

Is it OK if I start with the items mentioned in the table in Writing TRPL Rule-Sets, as these are methods implemented in ConfigLoader.java. Then I continue with features mentioned in Features section. After that, we'll see what else can be included.

Yes, that works.

Octavian Adrian Gavril wrote:

Eric, is Automatic Null Checking related to code sections similar to this?

[...]

Wow, this is some of the oldest code in FWD. I wrote that almost 20 years ago.

But yes, this implements the behavior that a boolean (sub-)expression which evaluates to null is treated as evaluating to false, so we can avoid NPEs being thrown from within TRPL expressions. One has to be aware of this behavior when writing those TRPL expressions.

If we want to get rid of this, the treatment of (branches of) logical expressions evaluating to null will need to be handled more explicitly somehow.

Greg Shah wrote:

Still looking for the Automatic Null Checking implementation to expand its documentation.

Eric: Can you please point us to the proper code?

I don't recall that well from so long ago, but I think the implementation is mostly in the Compiler.convertToRequiredType method (which Octavian already found). Look also at the Compiler.assembleNullTest method, though I think that may only be used for explicit null tests (not the implicit auto null-check).

It may be elsewhere as well. Review places where the IS_NULL and NOT_NULL constants are used (though these may be used for other purposes as well, which are not auto null-check related).

Updated the Expression Features documentation. I added an introduction to the classes used as tools to implement these features. Then I detailed each feature and described what the core implementation looks like. I have also included code sections for a better understanding of the behavior of the features.

Please, let me know if improvements are needed.

I committed the initial implementation of TRPL feature tracking system into task branch 1745a/15792. I added a class responsible for logging, another for handling the results in a map, and another class for deciding which features are critical or not.

I converted this file start.p:

message "Hello world!".

The csv output file looks like this:

Not sure if these results should differ from the converted project. Basically, I don't know if the conversion goes through all the rules regardless of the project.

Please take a look to see if I can go ahead with this.

Updated the Expression Features documentation. I added an introduction to the classes used as tools to implement these features. Then I detailed each feature and described what the core implementation looks like. I have also included code sections for a better understanding of the behavior of the features.

This is really good.

I committed the initial implementation of TRPL feature tracking system

These are interesting results. Some feedback:

1. We can't just run conversion, that will only load rules files that are used in that pipeline. This is most of the rule files, but not all. It would be missing things like the rules used for reports, database import, AST search, customer-specific rules and so forth.

Instead of relying upon conversion, we should dynamically lookup all .xml and .rules files in the pattern path and load them using your instrumented loader.

2. Some of the numbers are quite accurate while others are not obvious. For example, there aren't 8593 <while> elements in all our rules. The actual number is closer to 550. The number of <function instances is similarly incorrect (there should be around 1000 of them and instead nearly 70000 are reported). I suspect this comes from referencing some rules (e.g. common-progress.rules) from many places. This suggests 2 things:

• Once we've loaded a given file, don't add to our numbers when the file is processed again.
• Do we have an optimization opportunity here? Why would we load and compile the same rules file more than once? I think we should be able to reuse the same loaded code in many places, though we will have to ensure that we fix usage of "global data" such that we don't rely upon any variable references outside of the current file-level scope. I wonder what performance improvement we can get by this.

Observation: Features that can be removed: [<ast-spec>, <next-child-rules>]

ast-spec is a deprecated feature and definitely will be dropped.

next-child-rules is actually needed. 9 instances just means that only 9 rulesets use it, but those rules are quite helpful.

I've added some changes to avoid multiple uploads of the same .rules file. The numbers seem accurate, so I hope I've found the right places for this check.

I committed the changes in revision 15794.

Greg, as you suggested in the last call, I created a new task about that performance gap described in #1745-27.

Octavian Adrian Gavril wrote:

I reviewed ConfigLoader.java along with Writing_TRPL_Rule-Sets and these are some features that I found undocumented:

ATTRIBUTES	CONSTANT VALUES	FUNCTIONALITIES
execute-condition	view	parameter() doesn't extract init attribute
drop-condition	tree(no usage)	ruleSet(): depth is used but not documented
depth		ruleSet(): execute-condition is used but not documented
optional(ATTR_OPTIONAL/ATTR_OPT)		ruleSet(): drop-condition is used but not documented
namespace		ruleSet(): optional is used but not documented
mode(no usages)		worker(): namespace is used but not documented

Continue the same with Compiler and Writing_TRPL_Expressions.

Added properly documentation for parameters of rule-set/worker from Writing TRPL Rule-Sets.

Greg, what do you think about the numbers above? Also, what should I continue tracking with?

Please instrument expression.g and add those numbers to the results. This will tell us which expression features are used and how much.

Greg Shah wrote:

Greg, what do you think about the numbers above? Also, what should I continue tracking with?

Please instrument expression.g and add those numbers to the results. This will tell us which expression features are used and how much.

Sure!

Please try to confirm all of your numbers using other matching techniques (even something like grep). Many of the numbers don't seem correct. There are thousands of token NOT_EQ which is the text != but the numbers above say there are 11. There are thousands of CAST nodes (not 207) and even the next-child should be 8 not 7.

Greg Shah wrote:

Please try to confirm all of your numbers using other matching techniques (even something like grep). Many of the numbers don't seem correct. There are thousands of token NOT_EQ which is the text != but the numbers above say there are 11. There are thousands of CAST nodes (not 207) and even the next-child should be 8 not 7.

I think there might be some .rules files that are not referenced by any .xml files. dmo_common.rules, for example, has many uses of !=, but this file is not referenced. I'm working to fix this.

After some investigation, I believe the only way the TRPL expression can be compiled is by processing it together with a specific AST.

See PatternEngine.run(String profile):

         // Run global init rules.
         applyGlobal(RULE_INIT);

         Configuration config = Configuration.getInstance();

         // 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);
            }
         }

Greg, what do you think?

• In ConfigLoader.rule() I wanted to compile only the rule condition:
  

        // at this point, lastRule is the rule that was just created, now we
        // set whether this new rule is a WHILE or an IF/ELSE
        lastRule.setLoop(loop);
  
        new Expression(AstSymbolResolver.getResolver(), expr).execute();
  
        boolean oldInLoop = inLoop;

  
  Here I had some problems with using namespaces because they were not yet compiled.

• In ConfigLoader.loadImpl() I wanted to apply all the rules from lastRuleContainer after the load is finished:
  

           if (Objects.equals(suffix, SUFFIX_RULES))
           {
              lastRuleContainer.apply(AstSymbolResolver.getResolver(), RuleContainer.RULE_WALK);
              loadedRules.add(name);
           }

  
  Here I had some problems with rule type. In RuleContainer.apply(), each rule is applied with the same type. Therefore, compiling with only one rule type was not a good idea:
  

           RuleListElement next;
           for (int i = 0; i < size; i++)
           {
              next = rules.get(i);
              next.apply(resolver, type);
           }

In both cases, I get many NEP as there are some usage of CommonAstSupport.getSource() and CommonAstSupport.getCopy() are used during compiling.

I'm starting a new approach similar to Expression.main() to compile expression rules.

I think we will need a custom driver that loads everything (all rule set files) and forces all content to parse and all expressions to compile.

The table in #1745-44 was incomplete due to an issue while attempting to reformat the data into multiple tables. Apologies for the confusion. The table has now been updated in #1745-44.

I've done some quick checking of the results. Most of the numbers look pretty reasonable, even the ones that are very low.

For example, the bitwise operator counts look accurate (or at least close) as do the LTE, DIVIDE (maybe should be 9 not 8?).

On the other hand, CAST looks wrong (there should be around 2100) and LT seems wrong.

In addition to this, the matching in the expression.g rules is a problem because the counting is not done only for matches. It is unconditionally executed on exit from the rule. Since the expression precedence processing in ANTLR v2 requires nesting of the precedence levels in sepaarate rules, this leads to some rules getting very large counts which have no relation to the actual number of matches.

We are also missing counts for the keyword matching that happens here:

      keywords.put( "and",   Integer.valueOf(AND) );
      keywords.put( "false", Integer.valueOf(BOOL_FALSE) );
      keywords.put( "not",   Integer.valueOf(NOT) );
      keywords.put( "null",  Integer.valueOf(NULL) );
      keywords.put( "or",    Integer.valueOf(OR) );
      keywords.put( "true",  Integer.valueOf(BOOL_TRUE) );

I assumed that if logging occurs within the try block and no exception is thrown by match() up to that point, we could treat it as a valid match. I'm still exploring whether there's a more reliable approach.

You can still put the counter processing in the exit action, but you should only increment the count if the match has been made. The way to check this is to add a conditon like this:

un_expr
   :
      {
         String cls = null;
      }
      ( 
          m:MINUS^          { #m.setType( UN_MINUS ); }
        | BIT_COMP^
        | NOT^
        | h:HASH^ cls=cast! { #h.setType(CAST); #h.setText(cls); }
        | // empty alternative
      ) 
      primary_expr[false]
      {
         int type = (## == null) ? -1 : ##.getType();

         // HASH is counted in the cast rule
         if (TRPLFeatureTracker.trackerOn && (type == MINUS || type == BIT_COMP || type == NOT))
         {
            FeatureLogger.logFeatureUsage("RULE: un_expr");
         }
      }
   ;

This takes advantage of the fact that the root node for the returned AST can be referenced via ##.

Also, we don't need to track protected lexer rules since they don't actually generate tokens used by the parser. I'm not even sure we want to track the token generation itself, but instead would rather count things at the parser level.

Greg Shah wrote:

Also, we don't need to track protected lexer rules since they don't actually generate tokens used by the parser. I'm not even sure we want to track the token generation itself, but instead would rather count things at the parser level.

So, should I remove the logging of token rules?

Octavian Adrian Gavril wrote:

Greg Shah wrote:

Also, we don't need to track protected lexer rules since they don't actually generate tokens used by the parser. I'm not even sure we want to track the token generation itself, but instead would rather count things at the parser level.

So, should I remove the logging of token rules?

Yes, as long as you have logging for the "feature" at the parser level. For example, instead of the DIVIDE or MULTIPY tokens we should count the matches of those operators in the parser.

The idea here is that the language features match the parser structure, not the individual tokens.

• Added compilation support for <action> expressions. This improves logging for CAST and other problematic rules.
• Changed the caching of compiled expressions by using a more reliable key. (Considering column numbers might also be beneficial.)

I still need to address to this issue:

Greg Shah wrote:

In addition to this, the matching in the expression.g rules is a problem because the counting is not done only for matches. It is unconditionally executed on exit from the rule. Since the expression precedence processing in ANTLR v2 requires nesting of the precedence levels in sepaarate rules, this leads to some rules getting very large counts which have no relation to the actual number of matches.

I did some experiments but I need to continue.

I've also updated some .rules files that weren't compiling correctly due to syntax issues—specifically, rules/annotations/native_api.rules and rules/include/report.rules.

The new results look like this:

It's clear that there are still some unnecessary logs that need to be excluded—even for CAST. As for WS, its count is 0 because it's not directly referenced by any other rule.

Is AUTOMATIC NULL CHECKING really only used 5 times? That is suspicious.

I've completed some rules testcases covering all the expression/token/tag rules I monitor. These are not loaded in 1745a. Based on these findings, I have added some other changes to the tracker to make the results more accurate. I'm continuing with this approach. The most recent changes are committed into task branch 1745a/15801.

• avoided double logging of keywords from SYMBOL rule;
• turned off the tracking mode for included files to avoid double logging of function-library, include and worker tags;
• fixed reports/rpt_nullable_vars.xml syntax as this file generated some syntax errors (an action used inside work-rules);
• cleaned up expression.g.

• I found some rules that use data types from org.hibernate.*. These cause some files not to load.
• When CallGraphWorker there were some errors related to the failure to open the db connection with JanusGraphFactory.open:

    public static JanusGraph open(ReadConfiguration configuration, String backupName) {
        ModifiableConfiguration config = new ModifiableConfiguration(GraphDatabaseConfiguration.ROOT_NS, (WriteConfiguration)configuration, Restriction.NONE);
        String graphName = config.has(GraphDatabaseConfiguration.GRAPH_NAME, new String[0]) ? (String)config.get(GraphDatabaseConfiguration.GRAPH_NAME, new String[0]) : backupName;
        JanusGraphManager jgm = JanusGraphManagerUtility.getInstance();
        if (null != graphName) {
            Preconditions.checkNotNull(jgm, "Gremlin Server must be configured to use the JanusGraphManager.");
            return (JanusGraph)jgm.openGraph(graphName, (gName) -> new StandardJanusGraph((new GraphDatabaseConfigurationBuilder()).build(configuration)));
        } else {
            if (jgm != null) {
                log.warn("You should supply \"graph.graphname\" in your .properties file configuration if you are opening a graph that has not already been opened at server start, i.e. it was defined in your YAML file. This will ensure the graph is tracked by the JanusGraphManager, which will enable autocommit and rollback functionality upon all gremlin script executions. Note that JanusGraphFactory#open(String === shortcut notation) does not support consuming the property \"graph.graphname\" so these graphs should be accessed dynamically by supplying a .properties file here or by using the ConfiguredGraphFactory.");
            }

            return new StandardJanusGraph((new GraphDatabaseConfigurationBuilder()).build(configuration));
        }
    }

     [java] Generated unique-instance-id=c0a802c052490-aog1
     [java] Configuring index [search]
     [java] Initiated fixed thread pool of size 24
     [java] Configuring total store cache size: 5028244622
     [java] Gremlin script evaluation is disabled
     [java] Loaded unidentified ReadMarker start time 2025-04-28T13:58:04.321937Z into org.janusgraph.diskstorage.log.kcvs.KCVSLog$MessagePuller@24f3830c

All of these changes are committed into 1745a/15803.

I believe that generating/using callgraph is necessary to make rules from rules/reports and rules/callgraph load/compile correctly. Since this feature has not been used before, the error log contains new errors about other rule syntax that have not been loaded before.

• Fixed the usage of CallGraphWorker;
• Fixed logging for included files;
• Avoided getting type for variable and parameter to ignore the usage of deprecated classes;
• Cleaned up some .rules/.xml files which generated syntax errors;
• Avoided applying rules from PatternEngine.run because the copy/source ast is null.

I made some additional changes that fixed the way func-library children are processed. The main idea was to keep track of elements whether they are included or just loaded and take this into account the next time those elements appear. These changes are committed into 1745a/15805.

The current results are as follows:

I think the tracker is in a safe state now, because the results are pretty close to the real numbers. Tracking AUTO TYPE CONVERSION, AUTO-BOXING, AUTOMATIC NULL CHECKING and PROPERTY NOTATION is a bit more complicated than I thought. I used a TestResolver that can only recognize methods (including getters/setters) of type METH_GET_VAR. This way, all expression parsing completed successfully, but expression compilation did not. This happens because all variables or functions are seen as METH_GET_VAR and signature mismatches occur.

I posted some results for these features in the notes above, but all of them were due to the applyGlobal(RULE_INIT) and applyGlobal(RULE_POST) methods in PatternEngine.run. This applies the init/post rules in each .xml file. These method calls were ignored in 15804. PROPERTY NOTATION gets those results because its tracking is implemented in expression.g (independent of Compiler.process).

Greg, seeing these results, do you think this task can be safely paused?