Project

General

Profile

Feature #1735

leverage lambda expressions (Java 8 "closures") to reduce code bloat

Added by Eric Faulhaber over 11 years ago. Updated over 3 years ago.

Status:
WIP
Priority:
Normal
Assignee:
Start date:
Due date:
% Done:

80%

Estimated time:
400.00 h
billable:
No
vendor_id:
GCD

LambdaVsAnonymousInner.java Magnifier (2.33 KB) Greg Shah, 04/15/2016 10:06 AM


Related issues

Related to User Interface - Bug #3128: header expressions (and dynamic row/column/title...) may be executing more than is necessary New
Related to Base Language - Bug #3183: recursive use of some resources is sometimes broken New
Related to Base Language - Bug #3406: implement silent error mode as a lambda to allow aborting the control flow Closed
Related to Database - Support #4928: rewrite FieldReference to use lambdas instead of reflection New

History

#1 Updated by Eric Faulhaber over 11 years ago

Use anonymous methods rather than anonymous classes to implement the many features in the P2J runtime which require inversion of control. This should significantly reduce the amount of emitted code necessary to implement business logic callbacks.

#2 Updated by Greg Shah over 11 years ago

  • Target version set to Code Improvements

#3 Updated by Eric Faulhaber over 8 years ago

Instructions to upgrade to OpenJDK 8 on Ubuntu 15.04

To install the JDK and J2SE source (for debugging/review):

sudo apt-get install openjdk-8-jdk
sudo apt-get install openjdk-8-source

The installation should have made OpenJDK 8 the default JVM on your system (check java -version and javac -version). If not, run (at minimum) the following:
sudo update-alternatives --config java
sudo update-alternatives --config javac

In both cases, select the option for OpenJDK 8 (auto). If you had to make this change manually, please check the various other Java utilities to make sure these are using OpenJDK 8 as well. You can get the full list of Java-related alternatives with:
sudo update-alternatives --get-selections | grep java

Next, set up a default-java symbolic link in /usr/lib/jvm.

cd /usr/lib/jvm

If this symbolic link already exists, remove it:
sudo rm default-java

Create a new one for Java 8:
sudo ln -s java-8-openjdk-amd64 default-java

Change the /usr/lib/libjvm.so symlink as well:
cd /usr/lib
sudo rm libjvm.so
sudo ln -s /usr/lib/jvm/default-java/jre/lib/amd64/server/libjvm.so libjvm.so

If you have PostgreSQL installed locally for use with P2J, make sure the /etc/postgresql/9.<minor version>/<p2j cluster>/environment file references the default-java symlink for PL/Java's use. <minor version> is 1 for me, but 2 works as well. I think 3 has been used successfully, but I haven't tested it personally. If you followed our default P2J installation instructions, the <p2j cluster> name will be p2j_enUS.

The (non-commented portion of the) environment file should read as follows:

JAVA_HOME='/usr/lib/jvm/default-java'
CLASSPATH=''
LD_LIBRARY_PATH='$JAVA_HOME/jre/lib/amd64:$JAVA_HOME/jre/lib/amd64/server'

Finally, ensure the ldconfig cache will pick up the correct version of libjvm.so when building the P2J spawn tool. Run the following:
ldconfig -p | grep libjvm.so

You should see the following output:
libjvm.so (libc6,x86-64) => /usr/lib/libjvm.so

If this is not the only or the first entry, check the /etc/ld.so.conf.d directory for a configuration file related to a non-java-8 JVM. In my case, there was one named java-7-server.conf, which specified /usr/lib/jvm/java-7-openjdk-amd64/jre/lib/amd64/server. This resulted in the following precedence order in my ldconfig cache:
libjvm.so (libc6,x86-64) => /usr/lib/jvm/java-7-openjdk-amd64/jre/lib/amd64/server/libjvm.so
libjvm.so (libc6,x86-64) => /usr/lib/libjvm.so

That first entry shouldn't be there when running Java 8. It caused the spawn program to link to the Java 7 libjvm.so, which of course caused a problem spawning JVM clients compiled with Java 8. If you have a java-7-server.conf or similar config file which specifies a non-java-8 JVM, remove or otherwise disable it, then run
sudo ldconfig

to refresh the cache. You do NOT need to add a conf file for Java 8; the other changes above will ensure the correct libjvm.so is picked up. Building spawn should work now. Confirm with ldd spawn. The version of libjvm.so should be /usr/lib/libjvm.so:
    linux-vdso.so.1 =>  (0x00007ffc91f19000)
    libcrypt.so.1 => /lib/x86_64-linux-gnu/libcrypt.so.1 (0x00007fb8aa1f2000)
    libjvm.so => /usr/lib/libjvm.so (0x00007fb8a926a000)
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fb8a8ea0000)
    libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fb8a8b91000)
    libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fb8a8889000)
    libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fb8a8685000)
    libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fb8a8467000)
    /lib64/ld-linux-x86-64.so.2 (0x00007fb8aa42a000)
    libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fb8a8251000)

A note regarding PL/Java

At this time, we have not yet rebuilt PL/Java with Java 8. The version built with Java 7 continues to run without a problem under Java 8, and a p2jpl.jar file built under Java 8 will install and run properly in a database using a Java 8 runtime environment. We will investigate rebuilding PL/Java for Java 8 separately. If you need to retain the capability to build PL/Java from source, DO NOT remove Java 7 from your system until we have determined that it can be built with Java 8. However, DO follow the above steps to ensure OpenJDK 8 is the default for your system.

#4 Updated by Paul E over 8 years ago

For those of us on Ubuntu 14_04, the openjdk-8 packages don't exist (haven't been backported - seems unlikely that they will be).

Some options:
  • build OpenJDK from source
  • move to Ubuntu 15_04 (although we'll be making another move to 16_04 in April)
  • use the Oracle JDK
  • use Azul Zulu (OpenJDK built and hosted by Azul - I'd serve it from our binary repo)

In the very short term (today / tomorrow) I prefer using the Oracle JDK. I imagine in the demo timeframe we might look at using Azul Zulu.

#5 Updated by Eric Faulhaber over 8 years ago

Paul Eames wrote:

In the very short term (today / tomorrow) I prefer using the Oracle JDK.

FYI, I haven't done any runtime testing with the Oracle JDK, but I did build P2J with it. The default JavaDoc doclet in use seems a lot stricter than OpenJDK's default.

#6 Updated by Stanislav Lomany over 8 years ago

Addition to the instructions: p2jspi.jar should be copied to the new jre/lib/ext/.

#7 Updated by Paul E over 8 years ago

Does it matter that ldd spawn reports different dependencies for me than in your note 3?

My output:

ldd spawn 
    linux-vdso.so.1 =>  (0x00007fffb7dfe000)
    libcrypt.so.1 => /lib/x86_64-linux-gnu/libcrypt.so.1 (0x00007f38de329000)
    libjvm.so => /usr/lib/libjvm.so (0x00007f38dd34f000)
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f38dcf89000)
    libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f38dcc83000)
    libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f38dca7f000)
    libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f38dc860000)
    /lib64/ld-linux-x86-64.so.2 (0x00007f38de56b000)

#8 Updated by Greg Shah over 8 years ago

Interesting.

Eric's output had libstdc++.so which is the standard C++ class library. In other words, the ISO C++ standards specify a certain set of classes (e.g. string or vector) that all compliant implementations must provide. These are implemented in libstdc++.so. We don't write any C++ code, so I don't know why this is linked in to Eric's version.

https://gcc.gnu.org/onlinedocs/libstdc++/faq.html

Eric's output had libgcc_s.so which can be emitted on a machine-specific basis depending on CPU architecture. For example, it can provide some low-level math helpers that on some CPUs can be implemented directly using simple inline instructions. If Eric's CPU could not handle those cases, then these helpers might be required and the compiler would then emit calls to them that would be linked in via this library. On the other hand, I doubt Eric's CPU is incapable of these math features, so the more likely case is for exception handling. With the C++ library being included, the likely case is that the C++ exception handling pulled libgcc_s.so into the dependencies.

https://gcc.gnu.org/onlinedocs/gccint/Libgcc.html

Eric and I are both on Ubuntu 15.04. My ldd output is the same as his. With that said, if the spawner is working for you, then I would not worry about the missing libraries.

#9 Updated by Greg Shah almost 8 years ago

One thing I've been thinking about is that the lambda approach may have a positive affect on performance and resources. The idea is simply that the lambda approach has the potential to be more lightweight. There won't be heavyweight classes backing each lambda which is very different from our current anonymous inner class approach. This will reduce the (probably very large) memory pinned for all the anonymous Block (et al) instances.

I've also written a very naive test of the runtime performance of inner classes versus lambdas. I have attached it here. The results actually do show some promise.


public class LambdaVsAnonymousInner
{
   private static final long ITERATIONS = 10000000000L;

   int value1 = 0;
   int value2 = 0;

   public void loopInner(long num)
   {
      for (long i = 0; i < num; i++)
      {
         Worker<Object> worker = new Worker<Object>()
         {
            public void process(Object obj)
            {
               value1++;
            }
         };

         worker.process(null);
      }
   }

   public void loopInnerNoCapture(long num)
   {
      for (long i = 0; i < num; i++)
      {
         Worker<LambdaVsAnonymousInner> worker = new Worker<LambdaVsAnonymousInner>()
         {
            public void process(LambdaVsAnonymousInner me)
            {
               me.value1++;
            }
         };

         worker.process(this);
      }
   }

   public void loopLambda(long num)
   {
      for (long i = 0; i < num; i++)
      {
         Worker<Object> worker = (Object obj) -> { value2++; };
         worker.process(null);
      }
   }

   public void loopLambdaNoCapture(long num)
   {
      for (long i = 0; i < num; i++)
      {
         Worker<LambdaVsAnonymousInner> worker = (LambdaVsAnonymousInner me) -> { me.value2++; };
         worker.process(this);
      }
   }

   public static void main(String[] args)
   {
      LambdaVsAnonymousInner test = new LambdaVsAnonymousInner();

      long start = System.currentTimeMillis();

      test.loopLambdaNoCapture(ITERATIONS);

      long end = System.currentTimeMillis();

      System.out.printf("lambda no capture loop millis = %d\n", end - start);

      start = System.currentTimeMillis();

      test.loopLambda(ITERATIONS);

      end = System.currentTimeMillis();

      System.out.printf("lambda with capture loop millis = %d\n", end - start);

      start = System.currentTimeMillis();

      test.loopInner(ITERATIONS);

      end = System.currentTimeMillis();

      System.out.printf("inner class loop millis = %d\n", end - start);

      start = System.currentTimeMillis();

      test.loopInnerNoCapture(ITERATIONS);

      end = System.currentTimeMillis();

      System.out.printf("inner class loop no capture loop millis = %d\n", end - start);
   }

   private interface Worker<T>
   {
      public void process(T obj);
   }
}

The summary: if you use lambdas that access data from the surrounding context ("capturing lambdas"), then the performance is generally the same as an inner class implementation. If you rework the code to pass a reference through that allows the data to be accessed (non-capturing mode), then lambdas are actually significantly faster than inner classes. Inner classes don't gain any benefit from non-capturing mode.

lambda no capture loop millis = 5512
lambda with capture loop millis = 16327
inner class loop millis = 16386
inner class loop no capture loop millis = 16343

Ordering doesn't matter, the results are pretty consistent.

inner class loop millis = 16431
inner class loop no capture loop millis = 16391
lambda with capture loop millis = 16431
lambda no capture loop millis = 5464

I can't say if there is much to be gotten back in our server code using this non-capturing lambda approach. Profiling doesn't attribute much time to these methods. In order to implement this non-capturing mode, we would have to make sure that all scoped data was accessible in some instance that can be passed through all the block manager APIs so that when the lambdas are executed, their data reference can be passed.

Even the regular capturing lambdas will help from a resource perspective but it is not clear if the value we get is worth the implementation time right now.

#10 Updated by Greg Shah almost 8 years ago

A quick (and incomplete) list of the candidates for lambda conversion:

  • LogicalExpression
  • WhileClause
  • put_field (PutExpr)/export_field (ExportExpr) when these are complex expressions
  • BufferValidator
  • Validatable/Validator
  • complex form header expressions
  • complex embedded assignment expressions
  • complex browse column expressions
  • Block
  • UI Trigger
  • Database Trigger
  • ToClause
  • runtime_frame_options
  • runtime_browse_options
  • HandleExpr, func_in_handle_ref annotation
  • MESSAGE stmt parameter list
  • NO-ERROR
  • accumulate stmt
  • variable_def_anon_expr template usage
  • by clause with complex sort
  • client-side where (WhereExpression)
  • client where substitution
  • query substitution
  • RecordBuffer.start/endBatch()
  • FieldReference

#11 Updated by Greg Shah almost 8 years ago

Branch 1735a was merged to trunk as revision 11035. 1735a was archived.

This revision changes two cases of LogicalExpression usage in converted code into lambdas (deferred evaluation for right operand of OR or AND operator; WHILE clause in DO/FOR/REPEAT blocks). This eliminates a significant amount of generated code, leading to a cleaner/more readable result. It also removes a substantial number of anonymous inner classes (between 15% and 25% of classes in large projects). This update also removes non-compact mode support in the TRPL rule-sets.

1735a passed MAJIC conversion and runtime regression testing (only tc_job_clock_002 has failed multiple times, but that is a fragile test). The customer's GUI code also is passing conversion testing.

You should expect a VERY LARGE number of generated code changes for all converted code. Two examples:

OR/AND operators are much cleaner now:

<                   if (_or(isGreaterThanOrEqual(sel.getEndDate(), aDate), new LogicalExpression()
<                   {
<                      public logical execute()
<                      {
<                         return isUnknown(sel.getEndDate());
<                      }
<                   }))
---
>                   if (_or(isGreaterThanOrEqual(sel.getEndDate(), aDate), () -> isUnknown(sel.getEndDate())))

When such operators are combined in a single expression:

<                            if (_or(or(isGreaterThan(abs(date.differenceNum(period, dr.getDat())), 1), new LogicalExpression()
<                            {
<                               public logical execute()
<                               {
<                                  return and(and(cdays, new LogicalExpression()
<                                  {
<                                     public logical execute()
<                                     {
<                                        return isEqual(abs(date.differenceNum(period, dr.getDat())), 1);
<                                     }
<                                  }), new LogicalExpression()
<                                  {
<                                     public logical execute()
<                                     {
<                                        return or(isNotEqual(dr.getNum(), nextNum), new LogicalExpression()
<                                        {
<                                           public logical execute()
<                                           {
<                                              return isNotEqual(dr.getNum(), 1);
<                                           }
<                                        });
<                                     }
<                                  });
<                               }
<                            }), new LogicalExpression()
<                            {
<                               public logical execute()
<                               {
<                                  return and(and(cdays, new LogicalExpression()
<                                  {
<                                     public logical execute()
<                                     {
<                                        return isEqual(abs(date.differenceNum(period, dr.getDat())), 0);
<                                     }
<                                  }), new LogicalExpression()
<                                  {
<                                     public logical execute()
<                                     {
<                                        return isNotEqual(dr.getNum(), nextNum);
<                                     }
<                                  });
<                               }
<                            }))
---
>                            if (_or(or(isGreaterThan(abs(date.differenceNum(period, dr.getDat())), 1), () -> and(and(cdays, () -> isEqual(abs(date.differenceNum(period, dr.getDat())), 1)), () -> or(isNotEqual(dr.getNum(), nextNum), () -> isNotEqual(dr.getNum(), 1)))), () -> and(and(cdays, () -> isEqual(abs(date.differenceNum(period, dr.getDat())), 0)), () -> isNotEqual(dr.getNum(), nextNum))))

DO/REPEAT/FOR WHILE clauses:

<                doWhile("loopLabel3", new LogicalExpression(isOk), new Block()
---
>                doWhile("loopLabel3", () -> isOk, new Block()

Or if it is a more complex expression:

<             LogicalExpression whileClause0 = new LogicalExpression()
<            {
<               public logical execute()
<              {
<                  return isLessThan(startNum, endNum);
<               }
<            };
---
>            LogicalOp whileClause0 = () -> isLessThan(startNum, endNum);

In real applications I have seen the total number of converted classes be reduced by 15% to 25%. Although the raw performance of the implementation is roughly equivalent, it is expected that there may be benefit to the greatly reduced class loading, memory usage (and associated overhead).

#12 Updated by Greg Shah almost 8 years ago

Task branch 1735b has changes that modify conversion to emit UI validation expressions as lambdas instead of an inner class of the block. The runtime code is modified to accept lambdas. Validatable and Validator classes are now removed.

Both conversion and runtime regression (only tc_job_002 failed) testing passed. The converted code has significant expected changes. The following is an example.

Old:

         public void body()
         {
            editRangeRangeParmsFrame.widgetStartNum().setValidatable(new Validation0());
            ...
         }

         class Validation0
         extends Validator
         {
            public logical validateExpression(BaseDataType _newValue_)
            {
               final integer _startNum = (integer) _newValue_;
               return isGreaterThanOrEqual(_startNum, 0);
            }

            public character validateMessage(BaseDataType _newValue_)
            {
               return new character("Start of range must be non-negative.");
            }
         }

New:

editRangeRangeParmsFrame.widgetStartNum().setValidation(((ValidationExpr<integer>) (integer _startNum) -> isGreaterThanOrEqual(_startNum, 0)), "Start of range must be non-negative.");

There are 2 forms of the new CommonWidget.setValidation() method:

public void setValidation(ValidationExpr valexp, String valmsg);
public void setValidation(ValidationExpr valexp, ValidationMsg valmsg);

The first is used when the valmsg is a string literal. The second is used for all other cases.

Some validation expressions only access constants and the passed in new value. Such code will convert to a "naturally" non-capturing lambda. Such constructs are significantly faster than the original inner class approach. However, it is not clear to me if such a relative improvement can really be detected at the application level.

I've merged 1735b to trunk as revision 11038. Task branch 1735b has been archived.

There are significant changes to the manual MAJIC code. This is all checked in to git staging. On devsrv01, you must pull down the those MAJIC changes (cd ~/testing/majic && git pull origin staging). You must reconvert any 4GL code (devsrv01 or elsewhere) that has VALIDATION expression usage.

#13 Updated by Greg Shah over 7 years ago

Changed conversion to emit DELETE statement validation expressions as lambdas instead of an inner class of the block. Changed conversion to emit complex PUT or EXPORT expressions (not literals, not simple scalar var refs, not field references) as lambdas instead of an inner class of the block. In all 3 cases the runtime code is modified to accept and process lambdas. The BufferValidator class was removed. Refs #1735.

Both conversion and runtime regression (only a couple of common false negatives occurred) testing passed. The converted code has significant expected changes.

DELETE Validation Expressions

Old way:

         public void body()
         {
            RecordBuffer.openScope(book);
            book.create();
            book.deleteRecord(new Validation0());
         }

         class Validation0
         extends BufferValidator
         {
            public logical validateExpression()
            {
               return new logical(true);
            }

            public character validateMessage()
            {
               return new character("");
            }
         }

New way:

book.deleteRecord(() -> new logical(true), "");

You can see uast/delete_validation.p for a slightly more complicated version.

There are 2 forms of the new BufferImpl.deleteRecord() method:

public logical deleteRecord(Supplier<logical> valexp, String valmsg)
public logical deleteRecord(Supplier<logical> valexp, Supplier<character> valmsg)

The first is used when the valmsg is a string literal. The second is used for all other cases.

Complex PUT/EXPORT Expressions

Old:

               sStream.put(new FieldEntry[]
               {
                  new PutField(new PutExpr0(), FMT_STR_1),
                  ...
               });
            }

            ...
         }

         class PutExpr0
         extends GenericExpression
         {
            public BaseDataType resolve()
            {
               return subscript(cdat, i);
            }
         }

New:

               sStream.put(new FieldEntry[]
               {
                  new PutField(() -> subscript(cdat, i), FMT_STR_1),
                  ...
               });

PutField has been modified to add constructors (for each Resolvable constructor there is now also one that accepts a Supplier<BaseDataType>):

   public PutField(Supplier<BaseDataType> val)
   public PutField(Supplier<BaseDataType> val, String fmt)
   public PutField(Supplier<BaseDataType> val, boolean starts, int col)
   public PutField(Supplier<BaseDataType> val, boolean starts, NumberType col)
   public PutField(Supplier<BaseDataType> val, String fmt, boolean starts, NumberType col)
   public PutField(Supplier<BaseDataType> val, String fmt, boolean starts, int col)

This same approach is used for complex EXPORT expressions and the runtime changes are in ExportField. Any use of literals, simple scalar vars or field references are unchanged (which is most of the use cases). In MAJIC, this does drop about 2000 classes out of the application jar (roughly 7% of the business logic jar).

Some expressions only access constants and the passed in new value. Such code will convert to a "naturally" non-capturing lambda. Such constructs are significantly faster than the original inner class approach. However, it is not clear to me if such a relative improvement can really be detected at the application level.

I've merged 1735c to trunk as revision 11041. Task branch 1735c was archived.

There are significant changes to the manual MAJIC code. This is all checked in to git staging. On devsrv01, you must pull down the those MAJIC changes (cd ~/testing/majic && git pull origin staging). You must reconvert any 4GL code (devsrv01 or elsewhere) that has DELETE VALIDATION expression usage or which has complex PUT/EXPORT expressions.

#14 Updated by Paul E over 7 years ago

Greg, I really like the look of this.

#15 Updated by Greg Shah over 7 years ago

Thanks. Yes, it is already making a difference. There is much more improvement to come.

#16 Updated by Greg Shah over 7 years ago

Task branch 1735d is currently based from trunk 11041 (it needs a rebase to trunk 11044). As of 1735d branch rev 11044, the following features have been implemented:

1. Simple field references as the rvalue in an embedded assignment (SET/UPDATE statement) have been changed to emit as a FieldReference. The previous approach neglected this leaving a non-compilable result (and one that would not have worked properly even if it compiled).

set ...
    i = tt.num
    ...
    with frame f.

This used to convert to:

            FrameElement[] elementList0 = new FrameElement[]
            {
               ...
               new EmbeddedAssignment(i, tt.getNum()),
               ...
            };

            fFrame.set(elementList0);

Now it converts to this:

            FrameElement[] elementList0 = new FrameElement[]
            {
               ...
               new EmbeddedAssignment(i, new FieldReference(tt, "num")),
               ...
            };

            fFrame.set(elementList0);

This was a bug in the existing project.

2. Literals and simple variables as rvalues in embedded assignments are emitted directly instead of using a Resolvable. This is a simple code cleanup that has nothing to do with lambdas.

set i
    i = 3
    d = d2.

This used to convert to:

            FrameElement[] elementList0 = new FrameElement[]
            {
               new Element(i, frame0.widgeti()),
               new EmbeddedAssignment(i, new EmbeddedAssignmentExpr0()),
               new EmbeddedAssignment(d, new EmbeddedAssignmentExpr1())
            };

            frame0.set(elementList0);

            ...
         }

         class EmbeddedAssignmentExpr0
         extends GenericExpression
         {
            public BaseDataType resolve()
            {
               return new integer(3);
            }
         }

         class EmbeddedAssignmentExpr1
         extends GenericExpression
         {
            public BaseDataType resolve()
            {
               return d2;
            }
         }

Now it converts to:

            FrameElement[] elementList0 = new FrameElement[]
            {
               new Element(i, frame0.widgeti()),
               new EmbeddedAssignment(i, 3),
               new EmbeddedAssignment(d, d2)
            };

            frame0.set(elementList0);

3. Convert complex embedded assignment expressions to lambdas instead of inner classes.

set ...
    d = d2 * 5
    with frame f.

Used to convert to:

            FrameElement[] elementList0 = new FrameElement[]
            {
               ...
               new EmbeddedAssignment(d, new EmbeddedAssignmentExpr3())
            };

            fFrame.set(elementList0);
            ...
         }

         ...

         class EmbeddedAssignmentExpr3
         extends GenericExpression
         {
            public BaseDataType resolve()
            {
               return multiply(d2, 5);
            }
         }

Now it converts to:

            FrameElement[] elementList0 = new FrameElement[]
            {
               ...
               new EmbeddedAssignment(d, () -> multiply(d2, 5))
            };

            fFrame.set(elementList0);

4. Convert complex browse column expressions to lambdas instead of inner classes.

DEFINE BROWSE b1 QUERY q1
DISPLAY
        tt.f1
        tt.f2
        i * 30  /* complex expression will emit for deferred evaluation */
        tt.f3
        WITH 10 DOWN single.

This used to convert as:

            FrameElement[] elementList0 = new FrameElement[]
            {
               new Element(new FieldReference(tt, "f1"), f1Frame.widgetB1F1Column()),
               new Element(new FieldReference(tt, "f2"), f1Frame.widgetB1F2Column()),
               new HeaderElement(new ColumnExpr0(), f1Frame.widgetB1Expr4Column()),
               new Element(new FieldReference(tt, "f3"), f1Frame.widgetB1F3Column())
            };
            f1Frame.widgetB1().registerQuery(query0, elementList0);

            ...
         }

         class ColumnExpr0
         extends GenericExpression
         {
            public BaseDataType resolve()
            {
               return multiply(i, 30);
            }
         }

Now it converts as:

            FrameElement[] elementList0 = new FrameElement[]
            {
               new Element(new FieldReference(tt, "f1"), f1Frame.widgetB1F1Column()),
               new Element(new FieldReference(tt, "f2"), f1Frame.widgetB1F2Column()),
               new HeaderElement(() -> multiply(i, 30), f1Frame.widgetB1Expr4Column()),
               new Element(new FieldReference(tt, "f3"), f1Frame.widgetB1F3Column())
            };
            f1Frame.widgetB1().registerQuery(query0, elementList0);

5. Convert complex form header to lambdas instead of inner classes.

form header ...
            "Another " + txt format "x(14)" 
     with ...
          frame fr.

This used to convert to:

            FrameElement[] elementList0 = new FrameElement[]
            {
               ...
               new HeaderElement(new HeaderExpr0(), frFrame.widgetExpr3())
            };
            frFrame.registerHeader(elementList0);

            ...
         }

         class HeaderExpr0
         extends GenericExpression
         {
            public BaseDataType resolve()
            {
               return concat("Another ", txt);
            }
         }
            FrameElement[] elementList0 = new FrameElement[]
            {
               ...
               new HeaderElement(() -> concat("Another ", txt), frFrame.widgetExpr3())
            };
            frFrame.registerHeader(elementList0);

The runtime support classes (EmbeddedAssignment and HeaderElement) were modified to accept lambdas and to use them internally. With this change the are no more use cases of named inner classes that exist within the anonymous Block inner class instances (inner classes are still heavily used throughout the project, but named inner class members of blocks are now gone).

The resulting conversion in MAJIC and the customer GUI app causes substantial change and the elimination of thousands of inner classes.

This has passed MAJIC conversion and runtime regression testing (it took 2 passes, but only tc_job_002 was failing between the runs). CTRL-C testing has also passed. However, the current version does seem to cause MAJIC to run slower (the main regression runs in 210 to 215 minutes now, but before it was running in 195 minutes.

ETF has been at least partially tested using this build. Functionally it did not cause issues and there was no identifiable performance degradation.

Considering the potential performance impact on MAJIC, I am not planning to check this in at this time. I will do some further development work to eliminate the issue.

#17 Updated by Greg Shah over 7 years ago

Based on seeing the potential negative performance impact on MAJIC, I have done a less naive testcase to see if I can measure the performance problem using a converted 4GL testcase (instead of the naive hand-written testcase I've done in note 9.

def var txt as char init "NOT DISPLAYED".
def var i   as integer no-undo.
def var begin as int.
def var done  as int.

form header "A form header literal!" 
            space(30)
            "Another " + txt format "x(14)" 
     with title "Complex Header Expressions In a Loop" 
          no-labels
          down
          frame fr.
form i with frame fr.

def stream rpt.
output stream rpt to ./sample_form_header_report.txt.

txt = "Header".

begin = etime.

do i = 1 to 50000:
   display stream rpt i with frame fr.
   down stream rpt with frame fr.
end.

done = etime.

output stream rpt close.

message "Main loop executed in " + string(done - begin) + " milliseconds.".

I then ran this with 5 different conversion strategies.

1. Named Inner Classes of the Block (same as P2J Trunk)

            FrameElement[] elementList0 = new FrameElement[]
            {
               new HeaderElement("A form header literal!", frFrame.widgetExpr1()),
               new HeaderElement(new HeaderExpr0(), frFrame.widgetExpr3())
            };
            frFrame.registerHeader(elementList0);

            ...
         }

         class HeaderExpr0
         extends GenericExpression
         {
            public BaseDataType resolve()
            {
               return concat("Another ", txt);
            }
         }

2. Anonymous inner class

            FrameElement[] elementList0 = new FrameElement[]
            {
               new HeaderElement("A form header literal!", frFrame.widgetExpr1()),
               new HeaderElement(new GenericExpression()
               {
                  public BaseDataType resolve()
                  {
                     return concat("Another ", txt);
                  }
               }, frFrame.widgetExpr3())
            };
            frFrame.registerHeader(elementList0);

3. Capturing lambda

            FrameElement[] elementList0 = new FrameElement[]
            {
               new HeaderElement("A form header literal!", frFrame.widgetExpr1()),
               new HeaderElement(() -> concat("Another ", txt), frFrame.widgetExpr3())
               // new HeaderElement(() -> concat("Another ", "Header"), frFrame.widgetExpr3())
               new HeaderElement((Object[] args) -> concat("Another ", (character) args[0]), frFrame.widgetExpr3(), txt)
            };
            frFrame.registerHeader(elementList0);

4. Naturally non-capturing lambda (using literal text instead of a variable reference)

            FrameElement[] elementList0 = new FrameElement[]
            {
               new HeaderElement("A form header literal!", frFrame.widgetExpr1()),
               new HeaderElement(() -> concat("Another ", "Header"), frFrame.widgetExpr3())
            };
            frFrame.registerHeader(elementList0);

5. Non-capturing lambda (passing variable args to the HeaderElement which are then passed as lambda parameters to a refactored expression.

            FrameElement[] elementList0 = new FrameElement[]
            {
               new HeaderElement("A form header literal!", frFrame.widgetExpr1()),
               new HeaderElement((Object[] args) -> concat("Another ", (character) args[0]), frFrame.widgetExpr3(), txt)
            };
            frFrame.registerHeader(elementList0);

Results

Each set of numbers is (total elapsed time in ms for the test / total time spent executing the complex header expression in nanoseconds / average time spent executing the complex header expression in nanoseconds across 50000 iterations).

Scenario Fresh Server Run 1 Fresh Server Run 2 2nd Run Same Server 3rd Run Same Server
Named Inner Class (Trunk) 88980/53920187/1078 95224/56313942/1126 94395/50569822/1011 92950/50689991/1013
Anonymous Inner Class 95736/62658850/1253 89736/53308481/1066 92448/50329939/1006 89447/50506349/1010
Capturing Lambda 91797/59573074/1191 92839/60318190/1206 92035/46724485/934 84665/43076210/861
Naturally Non-Capturing Lambda (literal) 90752/38555104/771 96118/38255991/765 92368/29679006/593 87096/28971338/579
Non-Capturing Lambda (arguments passed) 91286/58270637/1165 88519/56464627/1129 92189/43115410/862 92355/46674317/933

Findings

  1. Although this test is less naive than the simple test in note 9, this latest attempt was NOT successful in duplicating any user-measurable change (in seconds of elapsed time) in the overall test execution based on the use of lambdas versus inner classes.
  2. The variability of the overall test execution time ranged from 84.7 seconds to 96.1 seconds.
  3. The fastest overall test execution time (84.7 seconds) occurred for the capturing lambdas (which is the slowest of the expression execution approaches).
  4. The slowest overall test execution time (96.1 seconds) occurred for the naturally non-capturing lambdas (which is the fastest of the expression execution approaches).
  5. The cumulative difference executing the expression over 50000 iterations was only 5.7 milliseconds on the Fresh Server Run 1 between the trunk approach of named inner classes (53920187 nanos or 53.9 ms) and the 1735d rev 11044 approach of capturing lambdas (59573074 nanos or 59.6 ms). In other words, when the overall variability of the testing may swing +/- 6 seconds, the difference in HeaderExpression implementation accounted for by capturing lambdas was not significant.
  6. Lambdas do amortize very well. If you leave the server running and hit that same code path again, the cost of each call will decrease such that lambdas are less expensive than inner classes.
  7. Non-capturing lambdas using args is pretty close (sometimes faster than) named inner classes.
  8. Naturally non-capturing lambdas are very fast, and are much faster (by up to 30 milliseconds cumulative over 50000 iterations) than all other cases including inner classes. But this is still so relatively small that it essentially cannot be seen by a user (it is far smaller than the natural execution variability of the test itself).

With just these results in mind, so far, I cannot justify reworking the lambda comversion to use the non-capturing arguments approach. Next I will try a much more heavily nested set of captured references mixed with a more complicated lambda expression. If I can't show any difference there, then the previous potential performance issue may have been bogus or may be something unrelated to lambdas.

#18 Updated by Greg Shah over 7 years ago

I have made an additional effort to create a testcase which could demonstrate some user-measurable performance degradation. After several rounds, I came up with this:


def var txt   as char init "NOT DISPLAYED".
def var i     as integer no-undo.
def var num   as integer no-undo. 
def var begin as int.
def var done  as int.

def stream rpt.
output stream rpt to ./sample_form_header_report.txt.

txt = "Header".

begin = etime.
repeat:
   do transaction:
      def var num2 as int init 100.
      repeat:
         do transaction:
            def var num3 as int init 200.
            repeat:
               def var num4 as int init 300.
               do transaction:
                  repeat:
                     num = 30.
                     repeat:
                        form header "A form header literal!" 
                                    space(30)
                                    "Another " + txt + string(num) + " " + string(num2) + " " + string(num3) + " " + string(num4)
                             with title "Complex Header Expressions In a Loop" 
                                  no-labels
                                  down
                                  frame fr.
                        form i with frame fr.

                        do transaction:
                           do i = 1 to 20000:
                              run loop-body.
                           end.
                        end.
                        leave.
                     end.
                     leave.
                  end.
               end.
               leave.
            end.
         end.
         leave.
      end.
   end.
   leave.
end.

done = etime.

output stream rpt close.

message "Main loop executed in " + string(done - begin) + " milliseconds.".

procedure loop-body:
   display stream rpt i with frame fr.
   down stream rpt with frame fr.
end.

Notice that this test has a very deeply nested set of anonymous Block instances and a complex expression that accesses multiple resources. Vars referenced in a form header are "promoted" to be members of the containing business logic class. But I manually edited the resulting code to move those vars into multiple different locations in the nested block hierarchy. In the result, I use System.nanoTime() to calculate the time spend executing lambdas. Although in my testcase, over 20000 iterations the capturing lambdas are slower than the original named inner class case, the difference is only measured in a cumulative tens of milliseconds. This cannot be seen at all in the overall elapsed time (the capturing lambda approach was "faster" than the named inner class approach, about half the time. The difference in overall timings is measured in hundreds of milliseconds to multiple seconds, depending on the run. The bottom line is that this result duplicates my findings from note 17, but it does so with a scenario that was meant to be as bad for the lambda case as is possible.

Based on this, I can't see any justification for using the non-capturing arguments approach at this time. I'm going to make some additional improvements to 1735d and then get it tested and merged to trunk.

#19 Updated by Greg Shah over 7 years ago

With revision 11050 of 1735d, I've made changes to reduce the unwrapping needed for the entered and not entered 4GL "builtin functions".

The old way:

            if ((frame0Frame.widgetStr().isEntered()).booleanValue())
            {
               ...
            }
            if ((frame0Frame.widgetStr().isNotEntered()).booleanValue())
            {
               ...
            }

New way:

            if (frame0Frame.widgetStr()._isEntered())
            {
               ...
            }
            if (frame0Frame.widgetStr()._isNotEntered())
            {
               ...
            }

It is just a minor improvement that occurs for control flow use cases.

#20 Updated by Greg Shah over 7 years ago

Branch 1735d was rebased from trunk 11051. The latest revision is now 11058 (it includes some revisions with changes for #3054).

I'm starting regression testing on this revision. Previous regression testing runs passed but the recent changes do require retesting.

#21 Updated by Greg Shah over 7 years ago

Regression testing for 1735d revision 11058 has passed.

#22 Updated by Greg Shah over 7 years ago

Branch 1735d has been rebased from trunk revision 11053. The latest revision is now 11060. Since there are no conflicts with the latest rebase, I am not going to re-run regression testing.

#23 Updated by Greg Shah over 7 years ago

Branch 1735d has been merged to trunk as revision 11054. 1735d was archived.

#24 Updated by Greg Shah over 7 years ago

Branch 1735e was created from P2J trunk revision 11071.

#25 Updated by Greg Shah over 7 years ago

Ovidiu:

In 1735e revision 11076, I am currently reworking all use of WhereExpression to replace it with a lambda (in this case, a Supplier<logical>).

This means that code that used to look like this (this is from testcases/uast/client-where-user-defined-function.p):

      externalProcedure(new Block()
      {
         WhereExpression whereExpr0 = new WhereExpression()
         {
            public logical evaluate(final BaseDataType[] args)
            {
               return isEqual(hotMess(somethinElse.getSomeTxt()), whateva.getNum());
            }
         };

         WhereExpression whereExpr1 = new WhereExpression()
         {
            public logical evaluate(final BaseDataType[] args)
            {
               return isEqual(hotMess(new character(valueOf(length(somethinElse.getSomeTxt())))), whateva.getNum());
            }
         };

         public void body()
         {
...

            forEach("loopLabel0", new Block()
            {
               CompoundQuery query0 = null;

               public void init()
               {
                  frame0.openScope();
                  query0 = new CompoundQuery(false, false);
                  query0.addComponent(new AdaptiveQuery(somethinElse, (String) null, null, "somethinElse.id asc"));
                  query0.addComponent(new AdaptiveQuery(whateva, (String) null, whereExpr0, "whateva.id asc"));
               }

               public void body()
               {
                  query0.iterate();

Now will look like this:

      externalProcedure(new Block()
      {
         public void body()
         {
...

            forEach("loopLabel0", new Block()
            {
               CompoundQuery query0 = null;

               public void init()
               {
                  frame0.openScope();
                  query0 = new CompoundQuery(false, false);
                  query0.addComponent(new AdaptiveQuery(somethinElse, (String) null, null, "somethinElse.id asc"));
                  query0.addComponent(new AdaptiveQuery(whateva, (String) null, () -> isEqual(hotMess(somethinElse.getSomeTxt()), whateva.getNum()), "whateva.id asc"));
               }

               public void body()
               {
                  query0.iterate();

We are no longer emitting the WhereExpression inner class and instead of using a reference like whereExpr0 we emit a lambda directly.

I need to update RuntimeJastInterpreter, to replace its usage of WhereExpression with the new lambda approach. My planned approach is to add a SupplierLogicalLambd@a subclass similar to @LogicalLambda and to use an annotation to detect the difference between LogicalOp and Supplier<logical> in the evalExpression() code that processes JavaTokenTypes.LAMBDA. Of course, I'll also remove references to WhereExpression and code for WhereExpressionAdapter. Any thoughts on my approach are welcome.

I need a testcase or testcases that would have previously generated the use of WhereExpression in dynamic queries. Can you point me to such a testcase? Without it I can't test my changes.

#26 Updated by Ovidiu Maxiniuc over 7 years ago

Please update the testcases repo. I just added uast/dynamic-queries/p2488-a.p. It contains 3 cases of queries that require at this moment WhereExpression internal classes. I usually put both static and dynamic form in order to know what to expect and compare codes at runtime.

The greatest difficulty will be to determine the correct interface for the interpreter. At this moment only two cases are handled: P2JQuery.Parameter if the cast is used and LogicalOp otherwise. Normally the parent signature should be checked and the interface obtained dynamically, but I estimate that this is too slow. If you can 'leave' some annotation with desired functional interface, that would be nice.

#27 Updated by Greg Shah over 7 years ago

Task branch 1735e was rebased from trunk rev 11082. The latest revision is now 11089.

#28 Updated by Greg Shah over 7 years ago

I have updated uast/dynamic-queries/p2488-a.p. The code did not run in the 4GL (there were both compile problems and runtime problems).

#29 Updated by Greg Shah over 7 years ago

I have checked in the final updates to finish client-where clause support. The dynamic query processing runtime conversion now supports the new lambda approach.

#30 Updated by Greg Shah over 7 years ago

Task branch 1735e was rebased from trunk rev 11083. The latest revision is now 11091.

#31 Updated by Greg Shah over 7 years ago

Task branch 1735e was rebased from trunk rev 11085. The latest revision is now 11093.

#32 Updated by Greg Shah over 7 years ago

With revision 11095, branch 1735e now converts the "emitdef" query substitution parameters into lambda expressions. The use case is for multi-table non-preselect queries, on subsequent components (not the first component) where there are complex expressions or case-insensitive character vars that must be resolved more than once. The first instance of such sub-expressions of a where clause were previously marked with an "emitdef" annotation and this caused them to emit as an inner class as part of the current top-level block's instance members (function, internal proc, trigger or external proc). Within that same top-level block, any duplicate use of that same sub-expression would reuse the same instance of the anonymous inner class rather than emitting again.

Although this is not hard to write a testcase that causes this behavior, in actual usage, this is not very common. A good example looks something like this (see testcases/uast/delegated_evaluation_of_query_terms.p):

def var num as int init 30 format "999".
def var txt as char init "good".
def var dat as date init today.

def temp-table tt-first
   field num1 as int format "999" 
   field txt1 as char
   field dat1 as date.

def temp-table tt-second
   field num2 as int format "999" 
   field txt2 as char    
   field dat2 as date.

create tt-first.
num1 = 30.
txt1 = "bad".
dat1 = today.
create tt-first.
num1 = 40.
txt1 = "bad".
dat1 = today.
create tt-first.
num1 = 50.
txt1 = "bad".
dat1 = today.
create tt-first.
num1 = 30.
txt1 = "good".
dat1 = today + 1.
create tt-first.
num1 = 40.
txt1 = "good".
dat1 = today  + 1.
create tt-first.
num1 = 50.
txt1 = "good".
dat1 = today + 1.
create tt-first.
num1 = 30.
txt1 = "good".
dat1 = today.
create tt-first.
num1 = 40.
txt1 = "good".
dat1 = today.
create tt-first.
num1 = 50.
txt1 = "good".
dat1 = today.

create tt-second.
num2 = 30.
txt2 = "bad".
dat2 = today.
create tt-second.
num2 = 40.
txt2 = "bad".
dat2 = today.
create tt-second.
num2 = 50.
txt2 = "bad".
dat2 = today.
create tt-second.
num2 = 30.
txt2 = "good".
dat2 = today + 1.
create tt-second.
num2 = 40.
txt2 = "good".
dat2 = today  + 1.
create tt-second.
num2 = 50.
txt2 = "good".
dat2 = today + 1.
create tt-second.
num2 = 30.
txt2 = "good".
dat2 = today.
create tt-second.
num2 = 40.
txt2 = "good".
dat2 = today.
create tt-second.
num2 = 50.
txt2 = "good".
dat2 = today.

for each tt-first where dat1 eq today and num1 > (num - 1) and txt1 eq txt and txt ne "",
    each tt-second where dat2 eq today and num2 > (num + 1) and txt2 eq txt and txt ne "":

   if num2 = 50 and dat2 = today then txt = if txt eq "good" then "bad" else "good".

   display num txt dat
           tt-first.num1  tt-first.txt1 tt-first.dat1
           tt-second.num2 tt-second.txt2 tt-second.dat2
           with frame f-bogus down.
   down with frame f-bogus.

end.

/* results */

/*
┌───────────────────────────────────────────────────────────────────┐
│num txt      dat      num1 txt1     dat1     num2 txt2     dat2    │
│─── ──────── ──────── ──── ──────── ──────── ──── ──────── ────────│
│030 good     08/23/16  030 good     08/23/16  040 good     08/23/16│
│030 bad      08/23/16  030 good     08/23/16  050 good     08/23/16│
│030 bad      08/23/16  040 good     08/23/16  040 bad      08/23/16│
│030 good     08/23/16  040 good     08/23/16  050 bad      08/23/16│
│030 good     08/23/16  050 good     08/23/16  040 good     08/23/16│
│030 bad      08/23/16  050 good     08/23/16  050 good     08/23/16│
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
│                                                                   │
└───────────────────────────────────────────────────────────────────┘

Procedure complete. Press space bar to continue.
*/

The previous conversion looked like this (look at the qSubExprXX members and how they are being used):

      externalProcedure(new Block()
      {
         ...

         DateExpression qSubExpr0 = new DateExpression()
         {
            public date execute()
            {
               return date.today();
            }
         };

         Int64Expression qSubExpr1 = new Int64Expression()
         {
            public int64 execute()
            {
               return plus(num, 1);
            }
         };

         CharacterExpression qSubExpr2 = new CharacterExpression()
         {
            public character execute()
            {
               return toUpperCase(txt);
            }
         };

         LogicalExpression qSubExpr3 = new LogicalExpression()
         {
            public logical execute()
            {
               return isNotEqual(txt, "");
            }
         };

         public void body()
         {
            ...

            forEach("loopLabel0", new Block()
            {
               CompoundQuery query0 = null;

               public void init()
               {
                  fBogusFrame.openScope();
                  query0 = new CompoundQuery(false, false);
                  query0.addComponent(new AdaptiveQuery(ttFirst, "ttFirst.dat1 = ? and ttFirst.num1 > ? and upper(ttFirst.txt1) = ? and ?", null, "ttFirst.id asc", new Object[]
                  {
                     (P2JQuery.Parameter) () -> date.today(),
                     minus(num, 1),
                     toUpperCase(txt),
                     isNotEqual(txt, "")
                  }));
                  query0.addComponent(new AdaptiveQuery(ttSecond, "ttSecond.dat2 = ? and ttSecond.num2 > ? and upper(ttSecond.txt2) = ? and ?", null, "ttSecond.id asc", new Object[]
                  {
                     qSubExpr0,
                     qSubExpr1,
                     qSubExpr2,
                     qSubExpr3
                  }));
               }

Here is the lambda version using rev 11095 of 1735e:

            forEach("loopLabel0", new Block()
            {
               CompoundQuery query0 = null;

               public void init()
               {
                  fBogusFrame.openScope();
                  query0 = new CompoundQuery(false, false);
                  query0.addComponent(new AdaptiveQuery(ttFirst, "ttFirst.dat1 = ? and ttFirst.num1 > ? and upper(ttFirst.txt1) = ? and ?", null, "ttFirst.id asc", new Object[]
                  {
                     (P2JQuery.Parameter) () -> date.today(),
                     minus(num, 1),
                     toUpperCase(txt),
                     isNotEqual(txt, "")
                  }));
                  query0.addComponent(new AdaptiveQuery(ttSecond, "ttSecond.dat2 = ? and ttSecond.num2 > ? and upper(ttSecond.txt2) = ? and ?", null, "ttSecond.id asc", new Object[]
                  {
                     (DateExpr) () -> date.today(),
                     (Int64Expr) () -> plus(num, 1),
                     (CharacterExpr) () -> toUpperCase(txt),
                     (LogicalExpr) () -> isNotEqual(txt, "")
                  }));
               }

There is a new set of Resolvable sub-interfaces that I have added to the persist package. These all omit the resolve() method but they each (CharacterExpr, DateExpr...) have a public default Class getType() method that makes each one the equivalent of a functional interface. This allows us to emit a well-typed lambda that properly implements Resolvable. The current implementations of CharacterExpression, DateExpression and so forth are adapter classes and thus they cannot be used as a functional interface.

By keeping to the Resolvable interface, the runtime did not need much change to match this new implementation. The only exception is that the dynamic query interpreter (RuntimeJastInterpreter) did require rework to handle these new lambdas. I also fixed some missing support for the ANON_CTOR cases but it is not actually clear if that code is even needed anymore. If it is really not needed, then we should delete it. The only thing I can think of that might still use it is the support for complex BY clauses, but I simply don't know if that code can be hit by the interpreter. I chose to be safe rather than sorry.

Eric/Ovidiu: I would appreciate if you would each do a code review of this branch. There is quite a bit of change in here, even if they don't actually get used in that many places in the real customer converted code.

By the way, I have tested the new support using standalone testcases and I cannot find any problems. I think that the result is functionally equivalent to the prior support.

#33 Updated by Ovidiu Maxiniuc over 7 years ago

Eric/Ovidiu: I would appreciate if you would each do a code review of this branch. There is quite a bit of change in here, even if they don't actually get used in that many places in the real customer converted code.

Greg, I reviewed the 1735e/11095. The changes related to dynamic queries are sound. There is a strange duplication of ___Expr / ___Expression classes. As you documented, the latter ones probably can be dropped as the old-fashioned WhereExpression are already dropped and at this moment, RuntimeJastInterpreter is only used for dynamic queries. I initially implemented the RuntimeJastInterpreter thinking to process all kind of JAST trees. In this case the anonymous ___Expression constructors would have the chance to be used. At this moment I don't think P2J will need such dynamic processor unless some strange feature is introduced in future versions of P4GL.
I understand that the new __Expr functional interfaces are to be used with lambdas of their respective types. BTW they all have a small typo in class javadoc (<<p>).
I also passed through all other changes. I think they are fine, in fact the TRPL code is simplified as the resulted generated one. I leave Eric have the last word here. I hardly wait the new code pushed to P2J trunk.

#34 Updated by Greg Shah over 7 years ago

BTW they all have a small typo in class javadoc (<<p>).

Thanks! It is now fixed.

#35 Updated by Eric Faulhaber over 7 years ago

Code review 1735e/11096:

This is an exciting improvement, even though the client-side where clause functionality (thankfully) isn't too common. The query substitution expression inner class stuff is more prevalent, and it will be good to have a tighter version of that in the converted code. That was some pretty impenetrable code you navigated. Nice work!

I didn't see anything concerning. I was a bit surprised how much complexity you found to remove in the where expression rules, though I guess that goes back to our discussions about what was really necessary to support in the current version of this feature set.

I didn't understand the TODO in control_flow.rules. I fixed a minor typo in database_access.rules, nothing functional, and committed it to rev 11097.

What still uses the WhereExpression class after these changes? The comment in the file header suggests there is still a use case.

#36 Updated by Greg Shah over 7 years ago

I didn't understand the TODO in control_flow.rules.

That is OK. :)

What still uses the WhereExpression class after these changes? The comment in the file header suggests there is still a use case.

It isn't needed. I have removed it in revision 11098.

#37 Updated by Greg Shah over 7 years ago

Rebased from trunk revision 11086. The latest revision is now 11099.

#38 Updated by Greg Shah over 7 years ago

Constantin:

I am trying to eliminate all usage of the defctorid (the annotation which allows us to target the anonymous Block instance's default inititializer. This is part of the steps I am taking to move our code blocks to a lambda model (#1735).

The frame_construction.rules implements a deferred frame construction using the defctorid as the location. This is done for all frames that have their frame_alloc node in an internal proc, function or trigger.

Your history entry related to this states:

** 011 CA  20131115          Static frames defined in an internal entry or trigger must be 
**                           instantiated when the internal entry/trigger executes.

I wrote a testcase that tries to leverage this "new frame created with every invocation" behavior. Here is what I came up with:

def var num as int.

def frame f-ext-proc num.
display num with frame f-ext-proc title "Ext Proc".

procedure int-proc:
   def frame f-int-proc num.
   display num with frame f-int-proc title "Int Proc".
end.

function func returns int ():
   def frame f-func num.
   display num with frame f-func title "Func".
   return num.
end.

on "f6" of num in frame f-ext-proc
do:
   def frame f-trig num.
   display num with frame f-trig title "Trigger".
end.

on "f7" of num in frame f-ext-proc
do:
   run int-proc.
end.

on "f8" of num in frame f-ext-proc
do:
   func().
end.

enable all with frame f-ext-proc.
wait-for go of frame f-ext-proc.

/* results if you type f6, f7, f8, f6, f7, f8 */

/*
┌─Ext Proc─┐
│       num│
│──────────│
│         0│
└──────────┘
┌─Trigger──┐
│       num│
│──────────│
│         0│
└──────────┘
┌─Int Proc─┐
│       num│
│──────────│
│         0│
└──────────┘
┌───Func───┐
│       num│
│──────────│
│         0│
└──────────┘
┌─Trigger──┐
│       num│
│──────────│
│         0│
└──────────┘
┌─Int Proc─┐
│       num│
│──────────│
│         0│
└──────────┘
┌───Func───┐
│       num│
│──────────│
│         0│
└──────────┘
*/

Question 1: Does this testcase (the resulting multiple instance display of the same frame) properly capture the behavior you were trying to implement?

The conversion results use the default initializer like this:

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFExtProc fExtProcFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFExtProc.class, "f-ext-proc");

   ...

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksIntProcFIntProc intProcFIntProcFrame = null;

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFuncFFunc funcFFuncFrame = null;

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksTrigger1FTrig trigger1FTrigFrame = null;

   /**
    * External procedure (converted to Java from the 4GL source code
    * in deferred_frame_instantiation_for_int_proc_func_and_trigger_blocks.p).
    */
   public void execute()
   {
      externalProcedure(new Block()
      {
         public void body()
         {
            ...

            fExtProcFrame.display(elementList0);

            EventList list0 = new EventList();
            list0.addEvent("f6", fExtProcFrame.widgetNum());
            registerTrigger(list0, TriggerBlock0.class, DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.this);

            ...
         }
      });
   }

   public void intProc()
   {
      internalProcedure(new Block()
      {
         {
            intProcFIntProcFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksIntProcFIntProc.class, "int-proc-f-int-proc");
         }
         public void body()
         {
            ...

            intProcFIntProcFrame.display(elementList1);
         }
      });
   }

   public integer func()
   {
      return function("func", integer.class, new Block()
      {
         {
            funcFFuncFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFuncFFunc.class, "func-f-func");
         }
         public void body()
         {
            ...

            funcFFuncFrame.display(elementList2);

            returnNormal(num);
         }
      });
   }

   public class TriggerBlock0
   extends Trigger
   {
      {
         trigger1FTrigFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksTrigger1FTrig.class, "trigger1-f-trig");
      }
      public void body()
      {
         ...

         trigger1FTrigFrame.display(elementList3);
      }

Question 2: Is the reason that we are using the default initializer instead of the Block.init() method that the calls to createFrame(), importSharedFrame() and createSharedFrame() need to happen before the BlockManager calls TransactionManager.pushScope()?

If my understanding is correct, I plan to implement something like this as a solution:

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFExtProc fExtProcFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFExtProc.class, "f-ext-proc");

   ...

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksIntProcFIntProc intProcFIntProcFrame = null;

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFuncFFunc funcFFuncFrame = null;

   DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksTrigger1FTrig trigger1FTrigFrame = null;

   /**
    * External procedure (converted to Java from the 4GL source code
    * in deferred_frame_instantiation_for_int_proc_func_and_trigger_blocks.p).
    */
   public void execute()
   {
      externalProcedure(new Block()
      {
         public void body()
         {
            ...

            fExtProcFrame.display(elementList0);

            EventList list0 = new EventList();
            list0.addEvent("f6", fExtProcFrame.widgetNum());
            registerTrigger(list0, TriggerBlock0.class, DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.this);

            ...
         }
      });
   }

   public void intProc()
   {
      intProcFIntProcFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksIntProcFIntProc.class, "int-proc-f-int-proc");

      internalProcedure(new Block()
      {
         public void body()
         {
            ...

            intProcFIntProcFrame.display(elementList1);
         }
      });
   }

   public integer func()
   {
      funcFFuncFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFuncFFunc.class, "func-f-func");

      return function("func", integer.class, new Block()
      {
         public void body()
         {
            ...

            funcFFuncFrame.display(elementList2);

            returnNormal(num);
         }
      });
   }

   public class TriggerBlock0
   extends Trigger
   {
      public void preScope()
      {
         trigger1FTrigFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksTrigger1FTrig.class, "trigger1-f-trig");
      }

      public void body()
      {
         ...

         trigger1FTrigFrame.display(elementList3);
      }

The idea is to place the create factory call into the code just above the BlockManager invocation, which should be logically the same execution state and order as the default initializer that will be executed during new Block() construction (which occurs before the BlockManager invocation).

As you can see, this is a problem for triggers, since the business logic does not instantiate those. The UI code does the instantiation on a delegated basis (this is needed for 4GL compatibility). The idea is to extend the Trigger class with a new method that is optionally emitted. When I move triggers to lambdas, I'll have to migrate that to its own lambda... but that is a problem for another day.

Question 3: Do you see any problems with my approach?

Question 4: Do you see any better way to do this?

#39 Updated by Greg Shah over 7 years ago

As an alternative, I could add the preScope() to Block and use a consistent approach for all block types. This has an added benefit of being more explicit about the dependencies for that code. That is probably the right way to go. As a down side, it means more lambda work later.

#40 Updated by Constantin Asofiei over 7 years ago

Greg Shah wrote:

Constantin:
Question 1: Does this testcase (the resulting multiple instance display of the same frame) properly capture the behavior you were trying to implement?

Correct. Another way to prove that each frame (defined in a trigger/procedure/function) is a new frame is to do a message frame f-int-proc:handle. after the display - a different handle ID will be shown for each invocation.

Question 2: Is the reason that we are using the default initializer instead of the Block.init() method that the calls to createFrame(), importSharedFrame() and createSharedFrame() need to happen before the BlockManager calls TransactionManager.pushScope()?

Correct.

Question 3: Do you see any problems with my approach?

For internal procedures/functions, the approach is OK. But why can't we do the same for triggers, too? Before they are invoked, a new instance is created (see TriggerDefinition.getTrigger), so it will be OK to initialize it in the trigger's default c'tor, no longer needing a preScope method. Or this contradicts the goal of removing the defctorid usage?

Question 4: Do you see any better way to do this?

For internal procedures/functions, no.

And as far as triggers go, we may need to emit two lambdas, one to execute any deferred initialization code (like frame creation), and one for the trigger's body.

#41 Updated by Greg Shah over 7 years ago

Or this contradicts the goal of removing the defctorid usage?

Yes, that's right. I am trying to get rid of all default initializer usage because when we switch Block usage to lambdas, there cannot be a default initializer.

And as far as triggers go, we may I to emit two lambdas, one to execute any deferred initialization code (like frame creation), and one for the trigger's body.

We will most likely have a lambda for each backing method used in the current TriggerBlock anon instance approach (pre, init, body...). I will setup the constructors to accept different combinations of these lambdas and will probably differentiate them using a no-parm, void returning functional interface that is used as a marker (instead of Runnable). This way we can cast the lambdas to the "correct" type and the proper constructor will be chosen. Otherwise we have to pass extra nulls all over the place to always have placeholders...

But I have quite a bit of additional work to get rid of all non-method usage in the Block code, before I can then work on emitting the methods as lambdas.

#42 Updated by Greg Shah over 7 years ago

Rebased task branch 1735e from trunk 11088. The latest revision is now 11102.

#43 Updated by Greg Shah over 7 years ago

Task branch 1735e revision 11103 has changes that replace all defctorid usage (and the corresponding default constructor for anonymous Block instances and trigger inner class defs) with a new preid annotation and a pre() method that gets invoked by the BlockManager before the TransactionManager.pushScope() call.

Diff output as an example:

*** old/src/com/goldencode/testcases/DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.java    2016-08-25 16:49:00.000000000 -0400
--- src/com/goldencode/testcases/DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.java    2016-08-30 14:49:43.798453044 -0400
***************
*** 65,73 ****
--- 65,75 ----
     {
        internalProcedure(new Block()
        {
+          public void pre()
           {
              intProcFIntProcFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksIntProcFIntProc.class, "int-proc-f-int-proc");
           }
+ 
           public void body()
           {
              intProcFIntProcFrame.openScope();
***************
*** 86,94 ****
--- 88,98 ----
     {
        return function("func", integer.class, new Block()
        {
+          public void pre()
           {
              funcFFuncFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksFuncFFunc.class, "func-f-func");
           }
+ 
           public void body()
           {
              funcFFuncFrame.openScope();
***************
*** 108,116 ****
--- 112,122 ----
     public class TriggerBlock0
     extends Trigger
     {
+       public void pre()
        {
           trigger1FTrigFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksTrigger1FTrig.class, "trigger1-f-trig");
        }
+ 
        public void body()
        {
           trigger1FTrigFrame.openScope();

There are many changes to the rules but the result in the generated code is surprisingly close to the old version.

#44 Updated by Greg Shah over 7 years ago

Rebased task branch 1735e from trunk 11093. The latest revision is now 11113.

#45 Updated by Greg Shah over 7 years ago

  • Estimated time changed from 80.00 to 400.00
  • % Done changed from 0 to 80

As of task branch 1735e revision 11119, external procedures, internal procedures, functions, inner blocks and editing blocks all emit in a lambda form. This represents all block types except for UI triggers and DB triggers (which will be handled in a separate branch). As one would expect, this requires major changes to the conversion and matching runtime changes to work with the new approach.

This was possible because starting with rev 11107, all other dependencies upon the anon inner class block structure had been reworked or eliminated, such that the final push for lambda block mode was feasible.

In addition to those big changes, the following smaller things were fixed (latent bugs in trunk):

  • The database_trigger_inner_class_def template had an ANON_CTOR/BLOCK node instead of a ANON_CTOR/CS_INSTANCE_VARS node. This caused a failure to convert certain DB trigger testcases (see testcases/uast/db_trig_sess1.p).
  • Fixed some processing in postprocess_find_query.xml which caused an abend when processing some dynamic queries (FIND-FIRST() or FIND-LAST()) because CS_INSTANCE_VARS were being processed assuming that that node was only present for the main class but in fact some use cases could have that node in an anon inner class. The postprocessing rules were made more restrictive and this abend is gone.
  • Fixed the rectangle-specific attributes to be properly implemented during conversion (the getters were not exposed properly and the setters were in the CommonWidget interface). EDGE-CHARS, EDGE-PIXELS, FILLED, GRAPHIC-EDGE, GROUP-BOX and ROUNDED attributes are now in the RectangleInterface and both conversion and the server-side runtime have been fixed.

The conversion is massively changed and will be hard to compare using diffs. The following will summarize the changes you should expect.

1. External procedures used to emit like this:

   public void execute()
   {
      externalProcedure(new Block()
      {
         public void body()
         {
            ...
         }
      });
   }

Now they emit like this:

   public void execute()
   {
      externalProcedure(ContainingExtProc.this, new Block((Body) () -> 
      {
         ...
      }));
   }

Things to note:

  • There is no longer an anon inner class of the abstract class Block. Instead, there is an instance of the concrete class Block and the code is emitted in (one or more) lambdas that are passed to the Block constructor.
  • The runtime used to be able to detect the containing business logic class using reflection. This was possible because the Block anon inner class was not static and thus it had an implicit reference to its containing class, which had an implicit reference to its containing class and so forth all the way up to the business logic outer class. The lambdas have no such linkage and so reflection does not help. By passing the class in as the first parameter to the externalProcedure() call, this is resolved without reflection. External procedures are the only case where this extr parameter is needed.
  • There are fewer lines of code.
  • There is 1 less level of indentation.

2. Internal procedures used to emit like this:

   public void someIntProc()
   {
      internalProcedure(new Block()
      {
         public void body()
         {
            ...
         }
      });
   }

Now it emits like this:

   public void someIntProc()
   {
      internalProcedure(new Block((Body) () -> 
      {
         ...
      }));
   }

3. User-defined functions used to emit like this:

   public integer hotMess(final character _txt)
   {
      return function("hot-mess", integer.class, new Block()
      {
         character txt = TypeFactory.initInput(_txt);

         public void body()
         {
            returnNormal(length(txt));
         }
      });
   }

Now they emit like this:

   public integer hotMess(final character _txt)
   {
      character txt = TypeFactory.initInput(_txt);

      return function("hot-mess", integer.class, new Block((Body) () -> 
      {
         returnNormal(length(txt));
      }));
   }

Notice how the local initialization of the INPUT mode parameter is done in the context of the block that contains the call to the BlockManager.function(). This will be explained further in item 7 below.

4. Editing blocks used to emit like this:

            updateEditing(fFrame, elementList0, "editingLabel0", new Block()
            {
               public void body()
               {
                  KeyReader.readKey();
                  apply(lastKey());
               }
            });

Now they emit like this:

         updateEditing(fFrame, elementList0, "editingLabel0", new Block((Body) () -> 
         {
            KeyReader.readKey();
            apply(lastKey());
         }));

5. Inner blocks in all their variants have changed.

DO blocks used to emit like this:

               doBlock(TransactionType.FULL, "blockLabel3", new Block()
               {
                  public void body()
                  {
                     ...
                  }
               });

Now they look like this:

            doBlock(TransactionType.FULL, "blockLabel3", new Block((Body) () -> 
            {
               ...
            }));

FOR EACH blocks emitted like this:

            forEach("loopLabel1", new Block()
            {
               AdaptiveQuery query0 = null;

               public void init()
               {
                  query0 = new AdaptiveQuery(tt, "tt.num2 >= 0", null, "tt.num2 asc");
               }

               public void body()
               {
                  /* picks index two */
                  query0.next();

                  ...
               }
            });

Now they look like this:

         forEach("loopLabel1", new Block((Init) () -> 
         {
            f0Frame.openScope();
            query0 = new AdaptiveQuery(tt, "tt.num2 >= 0", null, "tt.num2 asc");
         }, 
         (Body) () -> 
         {
            /* picks index two */
            query0.next();

            ...
         }));

Notice that the query0 is no longer defined in the block itself, it has been promoted to be a business logic class member. See item 7 below for details.

This example also shows that there can be multiple lambdas passed to the Block constructor. More details on that in item 6 below.

All the variants including the DO loops, REPEAT and FOR FIRST/LAST blocks have similar support.

6. The old anon inner class approach overrode Block default constructor and methods init(), enter(), body() and fini() as needed. With the new approach these 5 locations correspond to 5 differently typed lambdas, all of which are passed as parameters to the Block() constructor.

Old New
default constructor or pre() Pre
init() Init
enter() Enter
body() Body
fini() Fini

All of these new lambda types are functional interfaces that exist in com.goldencode.p2j.util. They take no parameters and return no value. In other words, they are the equivalent to Runnable. But they are named differently for 2 reasons:

  • It solves the problem (at compile time) that allows the Block() constructor to know the purpose of each lambda.
  • It allows the reader to understand at a glance the purpose of the contained code.

If a matching lambda exists for a particular type, it must always appear in the order shown in the table above (because that is the order expected by the various constructors for Block.

As noted below, it is possible to still use the anon inner class mode. If one does that, you will see that the default constructor is no longer used and has been replaced with a new method Block.pre() that is overridden. But in lambda mode you will see a lambda that is cast to Pre. These were primarily used for frame initialization in some top-level blocks (functions, internal procs and triggers) AND for query initialization. This was code that needed to execute within the TransactionManager scope of the containing block (which is where the default constructor got executed). Otherwise the code could have been placed in the init() method. Note that the BlockManager calls this hook before the TM.pushScope() is called (the init() and Init are called just after the TM.pushScope().

Old default constructor:

   public void intProc()
   {
      internalProcedure(new Block()
      {
         {
            intProcFIntProcFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksIntProcFIntProc.class, "int-proc-f-int-proc");
         }
         public void body()
         {
            intProcFIntProcFrame.openScope();

            ...
         }
      });
   }

New Pre lambda:

   public void intProc()
   {
      internalProcedure(new Block((Pre) () -> 
      {
         intProcFIntProcFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksIntProcFIntProc.class, "int-proc-f-int-proc");
      }, 
      (Body) () -> 
      {
         intProcFIntProcFrame.openScope();

         ...
      }));
   }

The Init and Body lambdas are pretty common and pretty well understood. But the enter() and Enter are more rarely seen. They only exist for use with FOR TO, FOR WHILE or FOR TO WHILE. Old way:

            forEachWhile("loopLabel4", () -> new logical(false), new Block()
            {
               AdaptiveQuery query3 = null;

               public void init()
               {
                  query3 = new AdaptiveQuery(tt, (String) null, null, "tt.num1 asc");
               }

               public void enter()
               {
                  query3.next();
               }

               public void body()
               {
                  ...
               }

            });

New way:

public class ForEachWhile
{
   ...

   AdaptiveQuery query3 = null;

   ...

   public void execute()
   {
      ...

         forEachWhile("loopLabel4", () -> new logical(false), new Block((Init) () -> 
         {
            query3 = new AdaptiveQuery(tt, (String) null, null, "tt.num1 asc");
         }, 
         (Enter) () -> 
         {
            query3.next();
         }, 
         (Body) () -> 
         {
            ...
         }));

The promotion of the query to be an instance member of the containing class will be explained in item 7 below.

The fini() or Fini case is only used for a FINALLY block. Old way:

            doBlock(TransactionType.FULL, "blockLabel1", new Block()
            {
               public void body()
               {
                  ...
               }

               public void fini()
               {
                  ...
               }
            });

New way:

         doBlock(TransactionType.SUB, "blockLabel0", onPhrase0, new Block((Body) () -> 
         {
            ...
         }, 
         (Fini) () -> 
         {
            ...
         }));

7. Queries, variables and some other resources were previously emitted into the CS_INSTANCE_VARS node of the anon inner class:

   public void ipFind()
   {
      internalProcedure(new Block()
      {
         integer v1 = TypeFactory.integer((long) 1);  // HERE
         public void body()
         {
            ...
         }
      });
   }

Resources in that location are inside the anon inner class but they initialize just before the BlockManager call is invoked since they initialize as part of the default initializer for the anon class.

There no longer is an anon inner class for each Block instance. These resources have been moved to one of two locations.

The first case is for resources that are effectively final (e.g. they are assigned once and never re-assigned), there is a new placeholder node created in the containing context (just above the BlockManager call). That placeholder does not emit itself, but rather it is just an attachment point that can be found using the instvarid annotation. The result looks like this:

   public void ipFind()
   {
      integer v1 = TypeFactory.integer((long) 1);  // HERE (just above the BlockManager call)

      internalProcedure(new Block((Body) () -> 
      {
         ErrorManager.silentErrorEnable();
         new FindQuery(t1, "t1.f1 = ?", null, "t1.id asc", new Object[]
         {
            v1
         }).next();
         ErrorManager.silentErrorDisable();
      }));
   }

The location is logically equivalent to the original approach.

The second case is for resources that are NOT effectively final. The only resource of that nature are queries. Please see the forEachWhile() example in item 6 above. Such resources were already uniquely named across the entire class, so they are now simply promoted to live as instance members of the containing outer (business logic) class. The result is allowed to be reassigned as needed and it is logically identical to the previous usage.

8. One less obvious consequence of moving to lambdas is the fact that the block of a lambda does not create a new variable naming scope but rather any local vars in that lambda are created in the naming scope of the containing method. Inner classes are different because the class itself and each contained method does create their own naming scope. Generally, this is not a problem because the Java representations of 4GL resources (variables/streams/frames/buffers/queries...) are already unique within naming scopes even with lambdas. The one known exception is for validation expressions. They always take a single parameter which is the new widget value being validated. We used to name that value by prepending an underscore to the lvalue name for the widget.

/home/ges/testing/majic/src/aero/timco/majic/xt/Textedit.java:36: error: variable _filename is already defined in method execute(logical,character,character,logical)
    [javac]          fTextFrame.widgetFilename().setValidation(((ValidationExpr<character>) (character _filename) -> notUnknown(FileSystemOps.searchPath(fTextFrame.getFilename()))), "ERROR: Text file not found!");

The problem with this is that this is the same approach that is used for procedure and function parameters. In the case that the widget name is the same as a parameter name, we get a conflict with lambdas that did not exist with the anon inner class approach.

For this reason, we now append the underscore for validation parameter names instead of prepending it (filename_ instead of _filename).

Some other useful notes:

  • This set of changes has a great impact on reducing the total number of lines of code. From the few places I have sampled, it seems to average about 15% fewer lines in the lambda approach.
  • The number of indentation levels are cut in half making it much easier to read deeply nested code (which is common).
  • Without the multitude of anon inner classes, the total number of classes in a project is greatly reduced. For MAJIC is went from 47956 classes to 28846 using lambdas. For the customer GUI project, it went from 227703 classes to 55835 classes.
  • The size of the application jars is massively reduced. In MAJIC, the majic.jar was 55,494,277 bytes and is now 29,223,234 bytes. In the customer GUI project, the <app_name>_gui.jar was 303,630,004 bytes and now it is 129,544,482 bytes.
  • It is possible to disabling the lambda block mode and convert with the anon inner classes. To do this add a lambdaBlock parameter to the p2j.cfg.xml and set it to false. I have not tested that mode, it is there for an easy fallback should we need it. I have not yet found a reason to use it. Please note that even in that mode the pre() method will emit now instead of the default constructor. Once we are sure we won't go back, that fallback mode will be removed.
  • I implemented all the runtime changes in a way that is backward compatible. This means that one could mix and match the anon inner classes approach with the lambda approach and there would be no issue. In fact, there may still be some hand-written code that follows the anon inner class model and that code will work.

Testing:

  • Manual conversion and runtime testing of a wide range of block types in standalone testcases has passed.
  • MAJIC conversion testing and runtime testing (both main and CTRL-C) has been passed.
  • The customer GUI app converted and compiled. I didn't see anything that was a problem in the converted code, but the changes are to extensive to review all of them.
  • The customer Server app converted and compiled.
  • Manual GUI standalone testcases for rectangle processing were tested (conversion and runtime) and that passed.

We are still waiting to see the ETF results for this branch. I am also still doing runtime testing of selected customer GUI scenarios.

If those both pass, I will merge this branch to trunk.

#46 Updated by Greg Shah over 7 years ago

  • File orchard_housing_gui_454_main_screen_p2j_trunk_11093_20160909.png added
  • File orchard_housing_gui_454_main_screen_p2j_after_click_on_debit_radio_button_1735e_11119_20160909.png added
  • File orchard_housing_gui_454_main_screen_initial_display_p2j_1735e_11119_20160909.png added

An issue was found in a customer's GUI app. See #3213 for details.

#47 Updated by Greg Shah over 7 years ago

Created task branch 1735f from P2J trunk 11093.

Task branch 1735e revisions 11118 and 11119 have been merged into 1735f. These changed represent fixes for rectangle-specific attribute support (conversion and server-side runtime). The changes were regression tested as part of 1735e and passed MAJIC conversion and runtime testing, conversion testing for the customer's Server and GUI projects and ETF testing. This change is needed to properly convert the latest level of the customer's GUI code.

Task branch 1735f was merged to trunk as revision 11094 and then was archived.

#48 Updated by Greg Shah over 7 years ago

I uncommitted the 11118 and 11119 revisions (rectangle-specific attributes) from 1735e. I then rebased 1735e from trunk 11094 (which added the rectangle-specific attributes support back in). The latest revision is now 11118.

The resulting branch is equivalent to the previous 1735e rev 11119 which passed most testing.

#49 Updated by Greg Shah over 7 years ago

Rebased 1735e from trunk 11095. The latest revision is now 11123.

Revision 11120 fixes the SharedVariableManager to properly support case-insensitive shared resources. This bug only can be seen when the legacy name is being used to share and import the resource. For this reason, it did not affect temp-tables which use a non-legacy name (created at conversion time). Shared variables were affected and are now fixed. I haven't checked which others were affected.

Revisions 11121 through 11123 remove the "always promote queries" and revise how they are initialized so that the local variables can be constructed as effectively final but the initialization can be deferred until the next scope is pushed. These changes fix the regressions in the customer GUI app, documented in note 46 and #3213. More details will be posted shortly.

#50 Updated by Greg Shah over 7 years ago

MAJIC regression testing passed and so did the ETF testing. The regression only manifests in the customer GUI app (see #3213). A temp-table named ttLscasubac is "new shared" by sc/Lscensb0.java. It is imported "shared" by sc/Lscenfn0.java.

I instrumented the SVM with some very verbose logging (of all Scopable notifications + specific outputs when ttLscasubac is added and removed from the SVM). I captured (in the server log) the output in both trunk and in 1735e rev 11119. I've placed the logs and instrumented SVM in your home dir on devsrv01 (see logs_from_1735e_bug_diagnosis.zip and svm_logging_version.zip).

The recreation is simple:

  • start the swing client
  • select the 454 testcase
  • in 1735e, by the time the User Defined Alerts window appears, the scope that contains the ttLscasubac table has already been removed
  • in trunk, it is never removed
  • at that point subsequent access rightly fails in 1735e
ges@ges:~/projects/<customer>/gui/run/server/logs$ cat trunk_log1 | grep "ttLscasubac" | wc -l
2749
ges@ges:~/projects/<customer>/gui/run/server/logs$ cat trunk_log1 | grep "ttLscasubac" | uniq
key = ttLscasubac, value = com.goldencode.p2j.persist.$__Proxy47@6c716fa8
--- --- --- --- --- --- --- --- --- ttLscasubac=com.goldencode.p2j.persist.$__Proxy47@6c716fa8

ges@ges:~/projects/<customer>/gui/run/server/logs$ cat 1735e_11119_log2 | grep "ttLscasubac" | wc -l
1947
ges@ges:~/projects/<customer>/gui/run/server/logs$ cat 1735e_11119_log2 | grep "ttLscasubac" | uniq
key = ttLscasubac, value = com.goldencode.p2j.persist.$__Proxy47@6a880285
--- --- --- --- --- --- --- --- --- ttLscasubac=com.goldencode.p2j.persist.$__Proxy47@6a880285
HERE HERE HERE! ttLscasubac SCOPE REMOVED!
[09/13/2016 11:30:54 EDT] (ErrorManager:SEVERE) {00000001:00000012:<customer>} Unable to locate shared temp-table or work-table definition for table ttLscasubac in procedure lscenfn0.p. (1429)

The removal in 1735e occurs at this point:

        at com.goldencode.p2j.util.SharedVariableManager.scopeDeleted(SharedVariableManager.java:1487)
        at com.goldencode.p2j.util.TransactionManager.processScopeNotifications(TransactionManager.java:5296)
        at com.goldencode.p2j.util.TransactionManager.popScope(TransactionManager.java:2015)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6804)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:294)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:268)
        at com.something.gui.common.Amndsfn0.execute(Amndsfn0.java:27)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.goldencode.p2j.util.ControlFlowOps$InternalEntryCaller.invokeImpl(ControlFlowOps.java:5395)
        at com.goldencode.p2j.util.ControlFlowOps$InternalEntryCaller.invoke(ControlFlowOps.java:5373)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:4434)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:3415)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:4111)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:4040)
        at com.goldencode.p2j.util.ControlFlowOps.invokeWithMode(ControlFlowOps.java:349)
        at com.goldencode.p2j.util.ControlFlowOps.invokeWithMode(ControlFlowOps.java:331)
        at com.something.gui.common.Aguitst0.lambda$null$14(Aguitst0.java:625)
        at com.goldencode.p2j.util.Block.body(Block.java:370)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6936)
        at com.goldencode.p2j.util.BlockManager.doBlockWorker(BlockManager.java:7877)
        at com.goldencode.p2j.util.BlockManager.doBlock(BlockManager.java:718)
        at com.something.gui.common.Aguitst0.lambda$launchGuiscreen$15(Aguitst0.java:610)
        at com.goldencode.p2j.util.Block.body(Block.java:370)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6936)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6727)
        at com.goldencode.p2j.util.BlockManager.internalProcedure(BlockManager.java:320)
        at com.goldencode.p2j.util.BlockManager.internalProcedure(BlockManager.java:306)
        at com.something.gui.common.Aguitst0.launchGuiscreen(Aguitst0.java:583)
...

The strange thing is that common.Amndsfn0 appears 5586 times in the trunk log. Based on my lambda changes, there should be exactly half the number of appearances in the 1735e log. Instead, we see 4217 entries and the last entry is the thing that causes the removal of the scope associated with the ttLscasubac temp-table (even though that scope was created by a completely different program).

I guess is that somehow the resource deletion processing is mismatched with my changes. I did have to make a change to bypass the execution of ProcedureManager.findRootEnclosingInstance() which uses reflection and relies upon the inner class structure of the Block anon inner class mode. It is only used now for non-lambda mode Block instances and with the TriggerDefinition helper class. I think that is OK, but something is wrong somewhere.

At this point, in addition to debugging sessions where I tried to work backwards from the failure, I got into a discussion with Constantin. He was able to duplicate the same problem, so it was not something environmental. He then posted this answer:

It finally hit me what is wrong with 1735e. You are promoting query variables as instance fields, instead of keeping them with the query loop. This gets problematic when the same persistent procedure has an internal proc/function invoked, and this appears one or more times in the stack. See this example:

def temp-table tt1 field f1 as int.

create tt1.
tt1.f1 = 10.
create tt1.
tt1.f1 = 20.

procedure proc0:
def input param i as int.

   for each tt1 where tt1.f1 > i:
      message tt1.f1.

      run proc0(tt1.f1 + 1).
   end.
end.

run proc0(9).

It should print 10,20,20. But P2J stops at 10,20. Why: on second invocation of proc0, the query0 (as is an instance field) is initiated again, and the previous query (from the stack) gets lost. When proc0 2nd invocation returns, it doesn't see the same query...

and this:

Further thinking, no procedure-specific resource can be emitted as an instance field for the external program, for the same reason as the for-each query.

From what I can see, we have problems only with frames and defined queries:


procedure proc0.
   def input param l as log.
   def buffer b for book.
   form l with frame f1.
   def query q for b.
   message l buffer b:handle frame f1:handle query q:handle.

   if l then run proc0(false).

   message l buffer b:handle frame f1:handle query q:handle.
end.

run proc0(true).

From this test, P2J emits as instance fields these:

   ResTestProc0F1 proc0F1Frame = null;

   QueryWrapper query0 = null;

They should be local to the internal procedure.

For resources (including queries and frames) which the 4GL scopes to a callable top-level block (functions, triggers, internal procedures), recursion is a problem if we promote those resources to be instance members of the containing class (which is equivalent to scoping for the external procedure in the 4GL).

Actually, we have existing problems in trunk with this (for both frames and queries). But in 1735e, I created a new problem because I was promoting all queries to be instance members. MAJIC and ETF testing did not use this "feature" in any case where they did recurse. The GUI app with its heavy ADM/ADM2 implementation was broken by this new promotion of all queries.

The problem to be solved here is that the query hierarchy was designed to configure most instances using parameters to the constructor. Some of the initialization logic that was being executed at that point has dependencies on running while inside the scope to which it is being attached. This means that we emitted these constructors in the init() method of the block inner class. This "split" definition approach worked fine in an inner class because what the compiler detects as a re-assignment of the query member is legal. In a lambda approach it makes them not effectively final and thus ineligible for reference from within a lambda. But we know that the initialization logic must be done inside the Init lambda.

In revisions 11121 through 11123, I have reworked the query hierarchy and the conversion of queries. We now construct the queries separately from where we instantiate them. We construct at the local instance variable location but we initialize in the Init lambda. Old approach:

            forEachWhile("loopLabel4", () -> new logical(false), new Block()
            {
               AdaptiveQuery query3 = null;

               public void init()
               {
                  query3 = new AdaptiveQuery(tt, (String) null, null, "tt.num1 asc");
               }

               public void enter()
               {
                  query3.next();
               }

               public void body()
               {
                  ...
               }

            });

New approach:

         AdaptiveQuery query3 = new AdaptiveQuery();

         forEachWhile("loopLabel4", () -> new logical(false), new Block((Init) () -> 
         {
            query3.initialize(tt, ((String) null), null, "tt.num1 asc");
         }, 
         (Enter) () -> 
         {
            query3.next();
         }, 
         (Body) () -> 
         {
         }));

All queries (except for FindQuery and AdaptiveFind) are now defined and instantiated at the same location. These cases always use a default constructor. The initialization logic is executed in the Init lambda using one of the initialize() method variants which match one to one with the overloaded constructors that used to exist (but which no longer exist).

You MUST always call initialize() after each default constructor usage and before any usage of the class (including calls to things like CompoundQuery.addComponent(). initialize() contains all the logic in the old constructors. We also return this from initialize() so that it can be chained with the constructor call. This is used for QueryWrapper.assign() and CompoundQuery.addComponent() idioms. Old way:

   final QueryWrapper query0 = new QueryWrapper("q", false);
   ...

      externalProcedure(new Block()
      {
         ...

         public void body()
         {
            query0.addBuffer(person, true);
            query0.addBuffer(persAddr, true);
            query0.addBuffer(address, true);
            f3Frame.openScope();
            RecordBuffer.openScope(person, persAddr, address);
            query0.assign(new CompoundQuery(true, false));
            query0.addComponent(new AdaptiveQuery(person, "?", null, "person.lastName asc, person.firstName asc, person.middleInit asc", "WHOLE-INDEX,name", new Object[]
            {
               (P2JQuery.Parameter) () -> isEqual(leading(), 2)
            }, LockType.NONE));
            query0.addComponent(new AdaptiveQuery(persAddr, "? = persAddr.empNum and upper(persAddr.linkType) = ? and ? and ?", null, "persAddr.linkType asc", "link-type", new Object[]
            {
               new FieldReference(person, "empNum"),
               toUpperCase(target),
               (P2JQuery.Parameter) () -> notUnknown(leading()),
               (P2JQuery.Parameter) () -> notUnknown(trimLeading())
            }, LockType.NONE));
            query0.addComponent(new AdaptiveQuery(address, "? = address.addrId and ?", null, "address.addrId asc", "addr-id", new Object[]
            {
               new FieldReference(persAddr, "addrId"),
               (P2JQuery.Parameter) () -> isEqual(leading(), 2)
            }, LockType.NONE));
            query0.open();
            query0.first();

New approach:

   final QueryWrapper query0 = new QueryWrapper("q", false);

   ...

      externalProcedure(ClientWherePromptQueryFor1.this, new Block((Init) () -> 
      {
         ClientWherePromptQueryFor1.this.target = target;
      }, 
      (Body) () -> 
      {
         query0.addBuffer(person, true);
         query0.addBuffer(persAddr, true);
         query0.addBuffer(address, true);
         f3Frame.openScope();
         RecordBuffer.openScope(person, persAddr, address);
         query0.assign(new CompoundQuery().initialize(true, false));
         query0.addComponent(new AdaptiveQuery().initialize(person, "?", null, "person.lastName asc, person.firstName asc, person.middleInit asc", "WHOLE-INDEX,name", new Object[]
         {
            (P2JQuery.Parameter) () -> isEqual(leading(), 2)
         }, LockType.NONE));
         query0.addComponent(new AdaptiveQuery().initialize(persAddr, "? = persAddr.empNum and upper(persAddr.linkType) = ? and ? and ?", null, "persAddr.linkType asc", "link-type", new Object[]
         {
            new FieldReference(person, "empNum"),
            toUpperCase(target),
            (P2JQuery.Parameter) () -> notUnknown(leading()),
            (P2JQuery.Parameter) () -> notUnknown(trimLeading())
         }, LockType.NONE));
         query0.addComponent(new AdaptiveQuery().initialize(address, "? = address.addrId and ?", null, "address.addrId asc", "addr-id", new Object[]
         {
            new FieldReference(persAddr, "addrId"),
            (P2JQuery.Parameter) () -> isEqual(leading(), 2)
         }, LockType.NONE));
         query0.open();
         query0.first();

Notice how the chaining is used in both the assign() and addComponent() cases.

FindQuery and AdaptiveFind are exceptions to this new rule. These cases always emit in-line in the code because they are not associated with a block header or loop header. For this reason, they can never be promoted and they cannot break recursion behavior. For these cases, the old constructor idiom remains the same.

This does fix the issue found in customer GUI app testing (note 46 and #3213). I have created a separate task to resolve the recursion issues that already exist in trunk, see #3183.

Revision 11123 has passed the following re-testing:

  • MAJIC conversion
  • Customer GUI app conversion and runtime scenario testing

MAJIC runtime testing is in process, as is the customer Server project conversion and ETF testing. If these pass I will be merging 1735e to trunk.

MAJIC did require some manual srcnew/ edits in order to match the latest 1735e revision 11123 approach. See #3180 for details.

#51 Updated by Greg Shah over 7 years ago

  • Related to Bug #3183: recursive use of some resources is sometimes broken added

#52 Updated by Greg Shah over 7 years ago

Rebased from trunk revision 11096. The latest revision is now 11125.

A regression was found in MAJIC runtime testing. That NPE was a conversion error and was resolved in revision 11125. Conversion and runtime regression testing is restarting.

#53 Updated by Greg Shah over 7 years ago

MAJIC conversion testing passed but runtime testing was broken (GSO 67 and GSO 133). Revision 11126 is meant to resolve this issue.

#54 Updated by Greg Shah over 7 years ago

Rebased 1735e from trunk revision 11097. The latest revision is now 11127.

#55 Updated by Greg Shah over 7 years ago

One more conversion fix was required to resolve a remaining problem with GSO 67 and GSO 133. This was made in rev 11128. After that, these tests now pass. GSO 156 failed but passed when run manually. There were some dc slot tests that failed but which were expected to fail at the time of day when they were run and they had passed recently. MAJIC regression testing has passed.

#56 Updated by Greg Shah over 7 years ago

Task branch 1735e was rebased from trunk revision 11098. The latest revision is now 11129.

This version has been tested with the customer GUI app scenarios and is working properly. There is no reason to repeat the MAJIC testing. At this time, we are only needing to get the customer Server project conversion and ETF test results to know if it is safe to merge to trunk.

Due to the very large number of updates being integrated into the trunk today and tomorrow, the plan is to let those all merge first and then to do a follow-up round of testing before 1735e merges to trunk.

#57 Updated by Greg Shah over 7 years ago

Task branch 1735e was rebased from trunk revision 11101. The latest revision is now 11132.

#58 Updated by Greg Shah over 7 years ago

Task branch 1735e was rebased from trunk revision 11102. The latest revision is now 11133.

#59 Updated by Greg Shah over 7 years ago

MAJIC regression testing has passed for 1735e revision 11133.

#60 Updated by Greg Shah over 7 years ago

Task branch 1735e was rebased from trunk revision 11105. The latest revision is now 11136.

#61 Updated by Greg Shah over 7 years ago

Merged 1735e to trunk as revision 11106 and sent notification email. Archived 1735e.

Created new task branch 1735g from trunk revision 11106.

#62 Updated by Greg Shah over 7 years ago

Created task branch 1735h from trunk 11106. There is a regression in RuntimeJastInterpreter exposed by the ETF testing. Igor came up with a fix and I have applied it to 1735h.

Eric is pushing this to the staging ETF pipeline and if that passes testing we will merge this to trunk.

#63 Updated by Greg Shah over 7 years ago

Igor's testing as posted in #3109-505 shows 1735h is safe.

1735h has been merged to trunk as revision 11107 and 1735h was archived.

#64 Updated by Greg Shah over 7 years ago

Task branch 1735g rebased from trunk revision 11107. The latest revision is now 11108.

#65 Updated by Greg Shah over 7 years ago

Igor has just asked a good question:

Do you know how to decipher lambda expressions' references in the call stack like this:
com.something.server.common.Aapisrch$$Lambda$332.body(),
com.something.server.common.Aapisrch.lambda$sPapiSrchCopyStatTtToResTt$18(handle, handle, handle, handle, handle, logical, integer) or
com.something.server.common.Aapisrch.lambda$null$17(handle, handle, handle, handle, integer, handle).
I've tried to find the information in the Internet but found nothing useful.

While I can't give you a complete answer, here are some things to know:

When a lambda is encountered at runtime, there is a multi-phase process by which the JVM prepares and executes the call site. The preparation is managed by a "meta-factory". The meta-factory is doing dynamic/runtime linkage of the call site to the functional interface and must also capture the references to the containing scopes. As part of this process, the runtime implementation of the call site is re-written. In the OpenJDK case, this meta-factory code can be found in
java/lang/invoke/*Metafactory.java:

java/lang/invoke/LambdaMetafactory.java (factory for meta-factories)
java/lang/invoke/AbstractValidatingLambdaMetafactory.java (abstract base class for a meta-factory)
java/lang/invoke/InnerClassLambdaMetafactory.java (concrete implementation of the meta-factory)

The InnerClassLambdaMetafactory creates an inner class structure for the lambdas, but it does so at runtime. This is the code that leads to the funny naming on the call stack.

The "$$Lambda" is just some hard coded text in InnerClassLambdaMetafactory, used as a separator. I think the "$332" is
just an ordinal specifying the which lambda in the compilation unit is being processed. The ".body()" is the method of the functional interface being executed, which is about the only useful thing here. I think you will have to leverage the line numbers to get anything useful from the stack trace.

In the second case, the "sPapiSrchCopyStatTtToResTt" is a reference to a method of Aapisrch.java. I suspect that the "(handle, handle, handle, handle, handle, logical, integer)" is a description of the captured
local variable references from the containing scope: handle ihResultTtbuf, handle ihStaticTtbuf, integer iiParentApiSeqNo, handle vhQuery, logical vlWorked, handle vhParentApiSeqNoField, handle vhApiSeqNoField. The difference in order is probably due to the lambda capturing being done in order of reference rather than the lexical order of appearance of the definitions. This last bit is pure supposition on my part, I haven't analyzed the meta-factory code enough to know for sure.

#66 Updated by Greg Shah over 7 years ago

Task branch 1735g rebased from trunk revision 11110. The latest revision is now 11114.

#67 Updated by Greg Shah over 7 years ago

Task branch 1735g rebased from trunk revision 11111. The latest revision is now 11117.

#68 Updated by Greg Shah over 7 years ago

1735g revision 11116 passed MAJIC conversion and runtime regression testing.

#69 Updated by Greg Shah over 7 years ago

Eric reports that 1735g rev 11117 passed customer Server project conversion and all ETF testing using the staging pipeline.

#70 Updated by Greg Shah over 7 years ago

1735g rev 11120 passed MAJIC conversion and runtime regression testing.

1735g rev 11121 passed MAJIC conversion testing and requires no runtime testing as it is conversion only and doesn't change MAJIC from 11120.

I am reconverting the customer GUI code with 11121 right now and assuming it passes (11120 did), I will manually regression test it. I expect it to work properly but won't know until sometime tomorrow.

Eric: Please push this to the staging pipeline for both conversion and ETF testing.

I will rebase this branch tomorrow morning. I don't plan to retest after rebase as there are no conflicts.

#71 Updated by Greg Shah over 7 years ago

1735g rev 11121 has passed customer GUI code conversion and runtime testing.

If the customer Server code conversion and ETF testing pass, then I will merge this branch to trunk.

#72 Updated by Greg Shah over 7 years ago

Task branch 1735g rebased from trunk revision 11113. The latest revision is now 11123.

#73 Updated by Eric Faulhaber over 7 years ago

Greg Shah wrote:

Eric: Please push this to the staging pipeline for both conversion and ETF testing.

1735g/11121 passed conversion and runtime ETF testing in the staging pipeline.

#74 Updated by Greg Shah over 7 years ago

Task branch 1735g rebased from trunk revision 11115. The latest revision is now 11125.

#75 Updated by Greg Shah over 7 years ago

Task branch 1735g has been merged to trunk as revision 11116 (refs #1735). This revision continues to shift the use of inner blocks into a lambda implementation.

The following describes the changes:

1. Additional blank lines will be seen before and after IF/THEN/ELSE, before and after CASE statements, before and after trigger registration and before WAIT-FOR.

2. The Block() class now allows its default constructor to be used, which is equivalent to a completely empty block (no lambdas passed means that none of the methods have an implementation). This is useful in scenarios seen in item 3 below.

3. Empty lambdas that were previously emitted, are now removed. This could happen for Body lambdas and also in the case where there was a completely empty block (previously it would emit with all 5 empty lambdas because the deletion code was only processed in an ascent rule and in a completely empty block there is no ascent because there are no children nodes).

So where you might see this previously:

         forEachWhile("loopLabel4", () -> new logical(false), new Block((Init) () ->
         {
            query3.initialize(tt, ((String) null), null, "tt.num1 asc");
         },
         (Enter) () ->
         {
             query3.next();
         },
         (Body) () ->
         {
         }));

Now you will see this:

         forEachWhile("loopLabel4", () -> new logical(false), new Block((Init) () ->
         {
            query3.initialize(tt, ((String) null), null, "tt.num1 asc");
         },
         (Enter) () ->
         {
            query3.next();
         }));

And something like this (an internal procedure that has no implementation in the 4GL) looked like this:

       internalProcedure(new Block((Pre) () ->
       {
       },
       (Init) () ->
       {
       },
       (Enter) () ->
       {
       },
       (Body) () ->
       {
       },
       (Fini) () ->
       {
       }));

And now it looks like this:

       internalProcedure(new Block());

4. The common case for EventList is now implemented as an inline constructor. The idea is that 99% of all usage of EventList is for a single OF clause. In these cases, the old way looked like this:

         EventList list0 = new EventList();
         list0.addEvent("x", fFrame.widgetCh());
         registerTrigger(list0, TriggerBlock0.class, TriggerLocalVar.this);

Constructors have been added to match all forms of the addEvent() method and the conversion now emits these parameters into the constructor when there is only a single OF clause (event_list OF widget_list). The new approach looks like this:

        registerTrigger(new EventList("x", fFrame.widgetCh()), TriggerBlock0.class, TriggerLocalVar.this);

For the more rare scenario where there are multiple OF clauses, the old separate EventList approach will still be used.

5. The GO-ON clause is now implemented as its own convenience sub-class of EventList, which enables the most common GO-ON cases to be implemented as an inline constructor. The old way looked like this:

          EventList list3 = new EventList();
          list3.addGoOn("enter");
          fFrame.update(elementList5, list3);

The new way:

          fFrame.update(elementList5, new GoOn("enter"));

6. UI triggers that do not have local variable definitions or local buffer definitions are now emitted as lambdas. The old way looked like this:

         EventList list0 = new EventList();
         list0.addEvent("f6", fExtProcFrame.widgetNum());
         registerTrigger(list0, TriggerBlock0.class, DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.this);
         EventList list1 = new EventList();
         list1.addEvent("f7", fExtProcFrame.widgetNum());
         registerTrigger(list1, TriggerBlock1.class, DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.this);
         ...
      }));
   }
   ...
   public class TriggerBlock0
   extends Trigger
   {
      public void pre()
      {
         trigger1FTrigFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksTrigger1FTrig.class, "trigger1-f-trig");
      }

      public void body()
      {
         trigger1FTrigFrame.openScope();

         FrameElement[] elementList3 = new FrameElement[]
         {
            new Element(num, trigger1FTrigFrame.widgetNum())
         };

         trigger1FTrigFrame.display(elementList3);
      }
   }

   public class TriggerBlock1
   extends Trigger
   {
      public void body()
      {
         ControlFlowOps.invoke("int-proc");
      }
   }

The new way:

         registerTrigger(new EventList("f6", fExtProcFrame.widgetNum()), DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.this, new Trigger((Pre) () ->
         {
            trigger1FTrigFrame = GenericFrame.createFrame(DeferredFrameInstantiationForIntProcFuncAndTriggerBlocksTrigger1FTrig.class, "trigger1-f-trig");
         },
         (Body) () ->
         {
            trigger1FTrigFrame.openScope();

            FrameElement[] elementList3 = new FrameElement[]
            {
               new Element(num, trigger1FTrigFrame.widgetNum())
            };

            trigger1FTrigFrame.display(elementList3);
         }));

         registerTrigger(new EventList("f7", fExtProcFrame.widgetNum()), DeferredFrameInstantiationForIntProcFuncAndTriggerBlocks.this, new Trigger((Body) () ->
         {
            ControlFlowOps.invoke("int-proc");
         }));

This will be the way that the vast majority of triggers will emit. The exception is when local resources are scoped to the trigger itself. This can be done for variables and buffers. In these cases, any recursive use of that trigger would possibly break in the lambda approach because the local resources would be a shared copy whereas in the 4GL, each trigger invocation would have its own instance of the variable or buffer. Our previous inner class implementation already handled this behavior properly. For this reason, we detect these cases and then use inner class mode instead of lambdas for the associated trigger. This is a pretty rare case and so most UI triggers will emit using lambdas.

Database triggers always use the inner class mode (they have not yet been migrated to lambdas).


The runtime has been written to be compatible with the prior implementation, so you should not need to reconvert immediately. When you do reconvert, you can expect a very large set of changes in MAJIC and customer GUI code in particular. There are some changes to customer Server code too, but it is less pervasive since the changes are to UI code.

Branch 1735g has been archived.

#76 Updated by Greg Shah over 7 years ago

Created task branch 1735i from trunk rev 11120.

#77 Updated by Greg Shah over 7 years ago

  • Status changed from New to WIP
  • Assignee set to Greg Shah

#78 Updated by Greg Shah over 7 years ago

  • Target version changed from Code Improvements to Converted Code Improvements - Deduplication

#79 Updated by Greg Shah over 7 years ago

  • File deleted (orchard_housing_gui_454_main_screen_initial_display_p2j_1735e_11119_20160909.png)

#80 Updated by Greg Shah over 7 years ago

  • File deleted (orchard_housing_gui_454_main_screen_p2j_after_click_on_debit_radio_button_1735e_11119_20160909.png)

#81 Updated by Greg Shah over 7 years ago

  • File deleted (orchard_housing_gui_454_main_screen_p2j_trunk_11093_20160909.png)

#82 Updated by Greg Shah over 6 years ago

Branch 3330a revision 11186 provides support for complex BY expressions emitted as lambdas.

#84 Updated by Greg Shah about 6 years ago

  • Related to Bug #3406: implement silent error mode as a lambda to allow aborting the control flow added

#86 Updated by Greg Shah over 3 years ago

We sometimes see converted code which causes a fatal javac problem "error: too many parameters". This can happen without any single explicit method call which has more than 255 parameters. The following is intended to explain what is happening.

The first thing to know about the "error: too many parameters" javac failure is that it is related to how many parameters are being passed to a method. Java allows up to 255 parameters to be passed to any method. Since instance methods have an implicit "this" parameter, there is a real limit of 254 parameters for instance methods. static methods can get the full 255 formal parameters.

Unfortunately, javac does not provide any diagnostic data at all for this error. There is no way to enable better data. So we must look at the code to see where the problem lies.

The second thing to know about this problem: it isn't always related to the formal parameter list provided in the source code.

The 4GL is described as a block structured language. It has a huge amount of implicit behavior encoded and reliant upon the block structure of the program, which can be (and often is) heavily nested. This affects transactions, sub-transactions, error processing, implicit retry of blocks, control flow, undo, infinite loop protection for retry, resource scoping, record locking, buffer release... an insane amount of hidden behavior.

In order to implement compatible behavior in FWD, we heavily delegate processing of the converted code to the FWD runtime. This is done by emitting the converted code as "anonymous methods". The common name for this is a "lambda". In Java, there is syntax to define a method which is a "chunk" of code that can be passed around or saved as data. It can be passed through a call chain down to some place in the FWD runtime and then called from that location zero or more times as needed.

Lambdas are special in many ways. One way is that they can directly reference local variables that are final or effectively final. Effectively final means a variable that is initialized/assigned once and then never re-assigned. These variables can live inside the method that contains the lambda(s). So these variables can be truly local data (not members of the enclosing class) and they can be referenced from these anonymous code chunks (methods) within that scope.

Each block in the 4GL creates at least one lambda in the converted code (for the block body itself) and possibly some other lambdas like one for initialization, catch blocks or for a finally block.

Nested blocks are also lambda, but they are nested inside containing lambdas which are ultimately nested inside some named method in the converted business logic class.

Since lambdas are really just an anonymous method, the java compiler (javac) has to do quite a bit of implicit processing to create these methods inside the bytecode (the compiled instructions in the "binary" class file). If the code inside a lambda references a local variable in the containing outer method, the compiler creates an implicit parameter for the lambda and passes that local variable reference along so that the chunk of code will see it and be able to reference it.

These implicit parameters "add up" because the deeply nested lambdas may be referencing local data which must be passed down through each containing lambda as implicit parameters. In other words, deeply nested references to local data get accumulated as implicit parameters so that at the top level of lambdas there can be a large number of parameters even if it is not obvious that this is the case.

Reducing the number of references or splitting the 4GL code into more than 1 method would solve the issue so long as it was no more than 254 local var references for that method.

#87 Updated by Greg Shah over 3 years ago

  • Related to Support #4928: rewrite FieldReference to use lambdas instead of reflection added

Also available in: Atom PDF