FWD » Core Development » Runtime Infrastructure

The idea was presented by Alex in the last statement from #9972-107. The general concept is to switch away from capturing lambdas in GenericWidget.getAttr() and GenericWidget.setAttr() and go with non capturing lambdas, such that the method usages would go from:

getAttr("frameRowOffset", () -> config.frameRowOffset, true)

getAttr("frameRowOffset", config, (c) -> c.frameRowOffset, true)

This should be seen strictly as a performance optimization.

This is an easy win. It is functionally the same and faster. There is good discussion of capturing/non-capturing lambdas in #6819.

As of trunk rev. 16279, there are 699 places which need refactoring for getAttr and 366 places for setAttr.

Alex: Should we dedicate 10858a for getAttr and 10858b for setAttr, or merge both of them in 10858a?

Alex: Should we dedicate 10858a for getAttr and 10858b for setAttr, or merge both of them in 10858a?

I think it is right to have them both in one branch so it gets easier to test.

Before starting the refactoring work, I'll like to lay out my plan and receive feedback in order to not waste time on implementing wrong solutions.

1. To avoid as much as possible the boxing/unboxing of values which could potentially cancel out this improvement, we need new functional interfaces for getAttr() and setAttr(). More exactly for: boolean, char, int, long, double.
   1. An example would be BooleanFunction:
      

      @FunctionalInterface
      public interface BooleanFunction<T>
      {
         boolean apply(T value);
      }

      
      So we would transform GenericWidget.getAttr() into:
      

      public <C extends WidgetConfig> boolean getAttr(BooleanFunction<C> fn, C config, boolean flush)
      {
         if (!configBatchingDisabled && flush)
         {
            getLogicalTerminal().flushEnqueuedWidgetAttrs();
         }
      
         return fn.apply(config);
      }

      
      Note the fact that I removed the first parameter, String fname, because I couldn't find any usages for it. Please correct this statement now if it's not right, as bringing back this parameter will require another round of exhaustive work for every getter usage.
   2. And also BooleanBiConsumer:
      

      @FunctionalInterface
      public interface BooleanBiConsumer<T>
      {
         void accept(T arg1, boolean arg2);
      }
      

      
      , which would transform GenericWidget.setAttr() into:
      

      public <C extends WidgetConfig> void setAttr(String fname,
                                                   boolean currentValue,
                                                   boolean newValue,
                                                   C config,
                                                   BooleanBiConsumer<C> setter)
      {
         if (currentValue == newValue)
         {
            return;
         }
      
         setter.accept(config, newValue);
      
         pushWidgetAttr(fname, newValue);
      }

2. We should do this modification for every getter and setter usage and then analyze its full performance potential. Alex suggested to restrict this to only 10-20 hotspots for a customer, but I think the time improvement would be too small to conclude something relevant.
3. Unfortunately, we can not use method references neither for setAttr() (because for example setHidden(boolean val) would not comply with the 2 parameters needed for a BooleanBiConsumer<T>), nor for getAttr() (because a getter has normally 0 arguments, meanwhile BooleanFunction<T> needs 1). So, we are left with stateless lambdas.

Note the fact that I removed the first parameter, String fname, because I couldn't find any usages for it. Please correct this statement now if it's not right, as bringing back this parameter will require another round of exhaustive work for every getter usage.

It may have been used in the base. I am afraid of having a decisive answer as I am not the author of this param. :/ Maybe somebody with more expertise can advice.

We should do this modification for every getter and setter usage and then analyze its full performance potential. Alex suggested to restrict this to only 10-20 hotspots for a customer, but I think the time improvement would be too small to conclude something relevant.

The customer for which this issue was created may have a scenario in which 10/20 such sets/gets consume 90% of the time. Maybe there are 50/60 or 100 gets/sets. It is fine. But all seem a bit too much ATM (before having some actual profilings and confirm the optimization).

Unfortunately, we can not use method references neither for setAttr() (because for example setHidden(boolean val) would not comply with the 2 parameters needed for a BooleanBiConsumer<T>), nor for getAttr() (because a getter has normally 0 arguments, meanwhile BooleanFunction<T> needs 1). So, we are left with stateless lambdas.

package test;

public class Test
{
   public static void main(String[] args)
   {
      Test test = new Test();
      getAttr(Test::getter, test, true);
   }

   public static <C> boolean getAttr(BooleanFunction<C> fn, C config, boolean flush)
   {
      return fn.apply(config);
   }

   @FunctionalInterface
   public interface BooleanFunction<T>
   {
      boolean apply(T value);
   }

   private boolean getter()
   {
      return false;
   }
}

this is automatically considered as the first parameter of the functional interface when using method references. The code above is well-typed.

Alexandru Lungu wrote:

It may have been used in the base. I am afraid of having a decisive answer as I am not the author of this param. :/ Maybe somebody with more expertise can advice.

Let's remove it, it has no use.

Alexandru Lungu wrote:

The customer for which this issue was created may have a scenario in which 10/20 such sets/gets consume 90% of the time. Maybe there are 50/60 or 100 gets/sets. It is fine. But all seem a bit too much ATM (before having some actual profilings and confirm the optimization).

Right, but how could we find such scenario? As far as I remember, in #9972 getters and setters took <1% in profiling. Wouldn't it take longer to profile the whole customer app searching for it, rather than just changing all usages, test and analyze the results? I did some benchmarks with non-capturing vs capturing lambdas in #9972 and I think for 100.000 usages the difference was down to a few milliseconds, so we shouldn't expect a big gain overall from these changes.

this is automatically considered as the first parameter of the functional interface when using method references. The code above is well-typed.

Ooh, right. It implies that this should be the actual config, correct? I went through most of the config classes and there are no getters/setters. All fields are public so there is no such encapsulation needed.

Ooh, right. It implies that this should be the actual config, correct? I went through most of the config classes and there are no getters/setters. All fields are public so there is no such encapsulation needed.

Correct. But this is a reason why getter/setter is an actual pattern: it allows referencing the setting and getting of a property using functional interfaces. I thing they are public mostly due to performance reasons and code clarity. But as far as I can tell, we already wrap a lot of setting an getting with setAttr and getAttr, so the "clarity" part is quite irrelevant already.

In other words, having getters and setters will be required.

Right, but how could we find such scenario? As far as I remember, in #9972 getters and setters took <1% in profiling. Wouldn't it take longer to profile the whole customer app searching for it, rather than just changing all usages, test and analyze the results? I did some benchmarks with non-capturing vs capturing lambdas in #9972 and I think for 100.000 usages the difference was down to a few milliseconds, so we shouldn't expect a big gain overall from these changes.

The performance overall is quite tricky to capture theoretically. The stateful lambdas create resources that are GC. It is not that is is slow to create, but also the stress on GC and the deallocation time is relevant - yet harder to profile.

As of trunk rev. 16279, there are 699 places which need refactoring for getAttr and 366 places for setAttr.

Overall I think this is a massive effort. The time involved may be allocated smarter into other optimizations from my POV. The goal of this task was to rather be incremental and general performance items to relate back to it and fix another dozens of getters/setters to fix the scenarios in question.

Alexandru Lungu wrote:

Ooh, right. It implies that this should be the actual config, correct? I went through most of the config classes and there are no getters/setters. All fields are public so there is no such encapsulation needed.

Correct. But this is a reason why getter/setter is an actual pattern: it allows referencing the setting and getting of a property using functional interfaces. I thing they are public mostly due to performance reasons and code clarity. But as far as I can tell, we already wrap a lot of setting an getting with setAttr and getAttr, so the "clarity" part is quite irrelevant already.

In other words, having getters and setters will be required.

Hmm, can you be more specific? Take this example from TreeListWidget:

public integer getTriggerColumn()
{
   return new integer(getAttr("triggerColumn", () -> treeListConfig().triggerColumn));
}

According to your statement, we should make TreeListConfig.triggerColumn private, make a public getter for it and pass it to getAttr()? Something like:

public integer getTriggerColumn()
{
   return new integer(getAttr(TreeListConfig::getTriggerColumn, treeListConfig()));
}

Is this correct?

Right, but how could we find such scenario? As far as I remember, in #9972 getters and setters took <1% in profiling. Wouldn't it take longer to profile the whole customer app searching for it, rather than just changing all usages, test and analyze the results? I did some benchmarks with non-capturing vs capturing lambdas in #9972 and I think for 100.000 usages the difference was down to a few milliseconds, so we shouldn't expect a big gain overall from these changes.

The performance overall is quite tricky to capture theoretically. The stateful lambdas create resources that are GC. It is not that is is slow to create, but also the stress on GC and the deallocation time is relevant - yet harder to profile.

As of trunk rev. 16279, there are 699 places which need refactoring for getAttr and 366 places for setAttr.

Overall I think this is a massive effort. The time involved may be allocated smarter into other optimizations from my POV. The goal of this task was to rather be incremental and general performance items to relate back to it and fix another dozens of getters/setters to fix the scenarios in question.

If it will be incremental, we'll have 2 versions of getters and setters (with capturing + non-capturing lambdas), but this shouldn't be a big deal. I'll profile the #9972 app and choose 20-30 getters and setters to modify.

According to your statement, we should make TreeListConfig.triggerColumn private, make a public getter for it and pass it to getAttr()? Something like:

Yes, with the exception that the field shouldn't be necessary private. It can be public and still have a getter just for the same of method referencing.

If it will be incremental, we'll have 2 versions of getters and setters (with capturing + non-capturing lambdas), but this shouldn't be a big deal.

This is fine. We have this in util or persist packages several times - we did this refactoring, but only in critical code (hit millions of times per request).

I'll profile the #9972 app and choose 20-30 getters and setters to modify.

This is what I mean by critical code. It is fine to have getAttr and setAttr overloaded to support both old and new pattern.

As of trunk rev. 16279, there are 699 places which need refactoring for getAttr and 366 places for setAttr.

Overall I think this is a massive effort. The time involved may be allocated smarter into other optimizations from my POV.

Maybe. I'd like to know an estimate of how much time it would take to solve the entire problem.

The goal of this task was to rather be incremental and general performance items to relate back to it and fix another dozens of getters/setters to fix the scenarios in question.

My worry here is that we'll spent more time figuring out which exact things to focus on than it would take to just fix this once.

Greg Shah wrote:

As of trunk rev. 16279, there are 699 places which need refactoring for getAttr and 366 places for setAttr.

Overall I think this is a massive effort. The time involved may be allocated smarter into other optimizations from my POV.

Maybe. I'd like to know an estimate of how much time it would take to solve the entire problem.

I already did a full getAttr() change with the old solution and it took me 8 hours. With the new solution for both getters/setters + javadocs I'm estimating 16 hours. Is this a reasonable time? If so, should we switch to the full implementation?

Yes, 16 hours is a reasonable time to complete the full task. Go ahead with that.

Hynek Cihlar wrote:

Code review.

BrowseColumnWidget.getLabelFgColor():
It reads labelBgColor via BrowseColumnConfig::getLabelBgColor instead of labelFgColor.

I know. I observed this but decided to not change as I thought it is intentional and the changes are not meant to be functional, only for performance. This is the trunk version:

public integer getLabelFgColor()
{
   int color = getAttr("labelBgColor", () -> config.labelBgColor);
   return (color == -1) ? new integer() : new integer(color);
}

Do you still want to change it?

BaseConfig:
Getter naming is inconsistent: getDcolor() uses a lower case c while the setter is setDColor().

Right.

BrowseWidget:setRowMarkers doesn't use the method reference pattern, is this expected?

It was due to an ambiguous method call. Java couldn't chose between using setAttr() with boolean or Boolean version. I'll fix it in the next commit.

Razvan-Nicolae Chichirau wrote:

Hynek Cihlar wrote:

Code review.

BrowseColumnWidget.getLabelFgColor():
It reads labelBgColor via BrowseColumnConfig::getLabelBgColor instead of labelFgColor.

I know. I observed this but decided to not change as I thought it is intentional and the changes are not meant to be functional, only for performance. This is the trunk version:

I see. This is clearly a bug. Please fix it. The changes will need to be regression tested anyway.

Hynek Cihlar wrote:

Razvan-Nicolae Chichirau wrote:

Hynek Cihlar wrote:

Code review.

BrowseColumnWidget.getLabelFgColor():
It reads labelBgColor via BrowseColumnConfig::getLabelBgColor instead of labelFgColor.

I know. I observed this but decided to not change as I thought it is intentional and the changes are not meant to be functional, only for performance. This is the trunk version:

I see. This is clearly a bug. Please fix it. The changes will need to be regression tested anyway.

Stanislav, please confirm.

Yes, this is incorrect:

public integer getLabelFgColor()
{
   int color = getAttr("labelBgColor", () -> config.labelBgColor);

Rebased 10858a to trunk rev. 16430 and commited rev. 16433 to fix new usages of the old setAttr() method.

• ChUI regression tests
• full UI suite from testcases project
• unit-tests from a customer
• GUI + ChUI smoke-tests on 3 customer apps + Hotel GUI
• Performance tests on 1 customer app

If this is enough, 10858a should be merged.

I won't put this in the merge queue yet. I would like to see if Hynek is ok with the change from #10858-26 + the testing plan first, as this seems to have a wider range of scenarios involved.

Code review 10858a. The changes look good.