Project

General

Profile

Bug #9019

reduce the usage of capturing lambdas in FWD runtime

Added by Constantin Asofiei almost 2 years ago. Updated almost 2 years ago.

Status:
Test
Priority:
High
Assignee:
Stefanel Pezamosca
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
production:
No
env_name:
topics:

Method-list--CPU.csv Magnifier (50.4 KB) Constantin Asofiei, 07/31/2024 04:43 AM

Method-list--CPU.csv Magnifier (49.9 KB) Constantin Asofiei, 08/05/2024 06:17 AM

History

#2 Updated by Constantin Asofiei almost 2 years ago

  • File Method-list--CPU.csvMagnifier added
  • Assignee set to Stefanel Pezamosca
  • Priority changed from Normal to High

Each capturing lambda in Java acts as a pseudo-anonymous class, and an instance of it is created on each call, to 'capture' the state on which to execute. In a #8363 app, it showed ~17 million lambda instances being created, from which ~10.8 million are from FWD runtime. The other are lambdas created from the converted code.

In YourKit profiler, this can be seen by filtering on java.lang.invoke.LambdaForm$MH.linkToTargetMethod to have them grouped by the lambda's parameter count and type, or via lambda*.<init> to show all instantiated lambdas. Attached is a report with all lambda invocations and their count, from com.goldencode package.

I've created branch 9019a from trunk rev 15360.

In rev 15361, there are some examples on how to remove the usage of capturing lambdas. Stefanel, please take a look at these changes, review the attached report and continue finding ways to reduce the capturing lambdas. We know (from previous performance work, like reducing BDT instances) that 'flooding' the runtime with lots of created instances is expensive, and reducing the capturing lambda count should help, too.

#3 Updated by Stefanel Pezamosca almost 2 years ago

  • Status changed from New to WIP

Sure, I will start working on this now.

#4 Updated by Stefanel Pezamosca almost 2 years ago

Constantin, would this be considered as an improvement ?

    static BaseRecord hydrateRecord(...)
    {
-      BaseRecord[] res = new BaseRecord[1];
-      HYDRATE_RECORD.timer(() -> res[0] = hydrateRecordImpl(resultSet, rowStructure, rsOffset, session));
-      return res[0];
+      BaseRecord res = null;
+      res = HYDRATE_RECORD.timerWithReturn(new ReturningOperation<BaseRecord>()
+      {
+         public BaseRecord exec()
+         throws PersistenceException
+         {
+            return hydrateRecordImpl(resultSet, rowStructure, rsOffset, session);
+         }
+      });
+      return res;
    }

It seems that SqlQuery.hydrateRecord() gets called a lot of times.

#5 Updated by Constantin Asofiei almost 2 years ago

No, that's still an instance created for the anon class. The point is to avoid instances being created (either via an explicit/anon class or lambda).

#6 Updated by Constantin Asofiei almost 2 years ago

Greg, adding a JVM-level argument to enable JMX lambdas may help (this would mean that depending on this flag we either call the JMX or directly the wrapped code). What I mean here only the JMX used for debugging, and not the JMX added for i.e. agent state.

#7 Updated by Greg Shah almost 2 years ago

Constantin Asofiei wrote:

Greg, adding a JVM-level argument to enable JMX lambdas may help (this would mean that depending on this flag we either call the JMX or directly the wrapped code). What I mean here only the JMX used for debugging, and not the JMX added for i.e. agent state.

Do you mean we should add our own custom property and use it at runtime to select the path of the code (lambdas or not)?

#8 Updated by Constantin Asofiei almost 2 years ago

Greg Shah wrote:

Constantin Asofiei wrote:

Greg, adding a JVM-level argument to enable JMX lambdas may help (this would mean that depending on this flag we either call the JMX or directly the wrapped code). What I mean here only the JMX used for debugging, and not the JMX added for i.e. agent state.

Do you mean we should add our own custom property and use it at runtime to select the path of the code (lambdas or not)?

Yes.

#9 Updated by Greg Shah almost 2 years ago

I'm OK with that (for the debugging cases). The default should be no lambdas.

#10 Updated by Stefanel Pezamosca almost 2 years ago

So, I can add something like:

private static final boolean JMX_DEBUG = System.getProperty("jmx.debug") != null;

Will be true if we add -Djmx.debug to JVM arguments.
Or something similar.

#11 Updated by Stefanel Pezamosca almost 2 years ago

  • % Done changed from 0 to 10
I committed to branch 9019a revision 15362:
  • I added the JMX_DEBUG flag in FwdServerJMX.java.
  • Skipped the HYDRATE_RECORD TimeStat call in SqlQuery.hydrateRecord when the flag is not set.
  • Removed some more lambda usages. Similarly with what was done for get() in FastFindCache, I have done for put().

#12 Updated by Greg Shah almost 2 years ago

Will be true if we add -Djmx.debug to JVM arguments.
Or something similar.

Let's name it -Dfwd_jmx_debug so that it won't ever conflict with Oracle's JMX properties.

#13 Updated by Stefanel Pezamosca almost 2 years ago

I committed to branch 9019a: (15363..15366)
  • Extracted processCommit and processRollback lambdas to processCommitImpl and processRollbackImpl methods in TransactionManager.
    Added the fwd_jmx_debug check for COMMIT and ROLLBACK jmx TimeStats.
  • Extracted byLegacy lambda to deregisterByLegacy() method in BufferManager.deregisterDynamicBuffer().
  • In RandomAccessQuery: extracted a lambda used for translateWhere to getTranslatedWhere() method reference.

Next on the list will be the BaseRecord class.

#14 Updated by Stefanel Pezamosca almost 2 years ago

  • % Done changed from 10 to 30

So, in BaseRecord the only capturing lambda use I've seen is this one in setActiveBuffer():

      RecordBuffer previousBuffer = activeBuffer;
      int previousOffset = activeOffset;
      Object propPreviousValue = activePropPreviousValue;
...
      return () ->
      {
         activeBuffer = previousBuffer;
         activeOffset = previousOffset;
         activePropPreviousValue = propPreviousValue;
      };
This is being used something like:
restoreActiveState = record.setActiveBuffer(RecordBuffer);
...
finally
{
   restoreActiveState.run()
}

I need a way to refactor the code so that it doesn't use Runnable for restoring. I looked at what Constantin did for SqlQuery.setUnknownMode and in AbstractQuery.
I'm thinking if I could do something similar, but in my case I need to return three variables, not just one.

Constantin, do you have any advice?

#15 Updated by Constantin Asofiei almost 2 years ago

Can setActiveBuffer ever be called while another call is active? I mean, currently we bracket this with setActiveBuffer and Runnable.run to restore, and if between these two calls setActiveBuffer can not be called again (for any record), then a singleton array, which is context-local in RecordBuffer, can be used to store and restore this data - i.e. setActiveBuffer gets this array as argument to store the previous state, and to restore call another BaseRecord.restoreActiveState API which restores the state.

#16 Updated by Stefanel Pezamosca almost 2 years ago

  • % Done changed from 30 to 60

For now, just to be on the safe side I made a new Runnable field to store and use it in case all previous values are null, -1 and null.

   private final Runnable restoreActiveState = () ->
   {
      activeBuffer = null;
      activeOffset = -1;
      activePropPreviousValue = null;
   };

This is because I see this comments.
    * A call to this method must be bracketed with a call to {@code run} the {@code Runnable} returned by
    * this method. This allows nested invocations and ensures that the active buffer and related state are
    * reset properly after the update work is done.
...
    * nested calls to set the active buffer are allowed

I committed last changes to 9019a revision 15367.

#17 Updated by Constantin Asofiei almost 2 years ago

Attached is the capturing lambda with 9019a/15367 and some changes I didn't yet commit.

#18 Updated by Stefanel Pezamosca almost 2 years ago

  • % Done changed from 60 to 70
  • Status changed from WIP to Review

I committed my last changes to 9019a revision 15368.

There were some changes before that were doing more bad than good and I modified them. Now it should be better.

I also fixed an error that caught my attention in the logs: FIX-CODEPAGE can only be run when the LONGCHAR variable is empty. This was thrown from longchar assignment to unknown value, because the current value was not empty here:

         // do not fix code page if already done
         if (!isCodePageFixed())
         {
            fixCodePage(this, (String) null);
         }
         super.assign(value);
So I moved the super.assign(value) before the call to fixCodePage().

Constantin, can you review the changes made until now?

#19 Updated by Stefanel Pezamosca almost 2 years ago

  • % Done changed from 70 to 80

I committed other changes to 9019a revision 15369.

About the usage of the new JMX_DEBUG flag: I think checking it just for some JMX TimeStats is enough. Doing this for some will reduce the readability of the code and it seems like a premature optimization.

Other thing is that comparing the performance difference between trunk and 9019a is not so accurate. I get different result everytime I run something. Sometimes 9019a is faster, but other times seems much slower, or the same as trunk.

#20 Updated by Constantin Asofiei almost 2 years ago

Stefanel, is 9019a in a good place to review/cleanup/test?

#21 Updated by Constantin Asofiei almost 2 years ago

I'm rebasing 9019a now.

#22 Updated by Constantin Asofiei almost 2 years ago

9019a 15369 was rebased from trunk rev 15375 - new rev 15384.

Stefanel, unless you have more pending fixes, pleas consider 9019a frozen. My plan is to test this on my machine, and once I have this (and my 8363h changes), I'll need your help to confirm my findings.

#23 Updated by Stefanel Pezamosca almost 2 years ago

Stefanel, unless you have more pending fixes, pleas consider 9019a frozen. My plan is to test this on my machine, and once I have this (and my 8363h changes), I'll need your help to confirm my findings.

Alright. At the moment I don't have any more pending fixes.

#24 Updated by Constantin Asofiei almost 2 years ago

9019a has some other changes:
  • rev 15386 - Inlined the SQLQuery logging, to avoid capturing lambdas.
  • rev 15385 - Some cleanup. More NanoTimer JMX conditional on the 'fwd_jmx_debug' flag, to avoid capturing lambdas.

Greg, I think this first phase can be put to test/review.

#25 Updated by Greg Shah almost 2 years ago

Code Review Task Branch 9019a Revisions 15376 through 15386

I'm good with the changes.

Eric: You need to review the persistence changes, especially the FastFindCache.

#26 Updated by Eric Faulhaber almost 2 years ago

Code Review 9019a/15376-15386 (persistence-specific):

I'm trying to remember why I structured the unknownModeRestore variable in AbstractQuery.preprocessSubstitutionArguments as a Runnable instead of a boolean. The change here looks ok to me, but please regression test the #5269 scenario, which was the cause for the nested query substitution parameter preprocessing logic which now has changed in this branch.

The FastFindCache code in this branch is out of date. There were conflicting changes in 6229b, which was merged to trunk rev 15376. The changes in 9019a will have to be reworked after a rebase.

In RecordBuffer.defineReadOnly, the duplicated block of code is long enough that it probably should be refactored into a separate method.

In TriggerTracker.getTracker, are we sure that replacing computeIfAbsent with get and put is actually faster? This seems like a possible anti-pattern to me, as it doubles the hash map lookup work.

All the other persistence updates look ok to me.

#27 Updated by Constantin Asofiei almost 2 years ago

Eric Faulhaber wrote:

I'm trying to remember why I structured the unknownModeRestore variable in AbstractQuery.preprocessSubstitutionArguments as a Runnable instead of a boolean. The change here looks ok to me, but please regression test the #5269 scenario, which was the cause for the nested query substitution parameter preprocessing logic which now has changed in this branch.

The runnable was just a restore mechanism, with a simple boolean assignment. Having this return the previous boolean value and use that in restore is the same logic. I'll check #5269

The FastFindCache code in this branch is out of date. There were conflicting changes in 6229b, which was merged to trunk rev 15376. The changes in 9019a will have to be reworked after a rebase.

Rebased 9019a from trunk rev 15376 - new rev 15389. Rebase fixes in rev 15389.

In RecordBuffer.defineReadOnly, the duplicated block of code is long enough that it probably should be refactored into a separate method.

Done in rev 15388.

In TriggerTracker.getTracker, are we sure that replacing computeIfAbsent with get and put is actually faster? This seems like a possible anti-pattern to me, as it doubles the hash map lookup work.

The idea is getTracker is called 500k times in the #8363 scenarios, with 500k capturing lambdas instantiated for each call. The get/put avoids this.

#28 Updated by Constantin Asofiei almost 2 years ago

Constantin Asofiei wrote:

Eric Faulhaber wrote:

I'm trying to remember why I structured the unknownModeRestore variable in AbstractQuery.preprocessSubstitutionArguments as a Runnable instead of a boolean. The change here looks ok to me, but please regression test the #5269 scenario, which was the cause for the nested query substitution parameter preprocessing logic which now has changed in this branch.

The runnable was just a restore mechanism, with a simple boolean assignment. Having this return the previous boolean value and use that in restore is the same logic. I'll check #5269

I don't see anything wrong with #5269.

#29 Updated by Eric Faulhaber almost 2 years ago

I looked at FastFindCache after the rebase/rework. The changes look ok to me. Is there a reason the same type of capturing lambda reduction was not needed for the invalidate method variants?

#30 Updated by Stefanel Pezamosca almost 2 years ago

Eric Faulhaber wrote:

I looked at FastFindCache after the rebase/rework. The changes look ok to me. Is there a reason the same type of capturing lambda reduction was not needed for the invalidate method variants?

There wasn't really a specific reason, so I guess we could do the same for invalidate also ?

#31 Updated by Stefanel Pezamosca almost 2 years ago

Eric Faulhaber wrote:

In TriggerTracker.getTracker, are we sure that replacing computeIfAbsent with get and put is actually faster? This seems like a possible anti-pattern to me, as it doubles the hash map lookup work.

Actually computeIfAbsent does exactly the same thing, but with the extra step of using a Function to create a new instance. I was looking at the wrong implementation.

#32 Updated by Stefanel Pezamosca almost 2 years ago

I committed to 9019 revision 15390 capturing lambda reduction changes for invalidate method in FastFindCache.

#33 Updated by Constantin Asofiei almost 2 years ago

Stefanel Pezamosca wrote:

I committed to 9019 revision 15390 capturing lambda reduction changes for invalidate method in FastFindCache.

Please make the invalidateImpl methods private. Otherwise, the changes are ok (invalidate is being called ~500k times, so lambda reduction is good).

#34 Updated by Stefanel Pezamosca almost 2 years ago

  • % Done changed from 80 to 90

Constantin Asofiei wrote:

Please make the invalidateImpl methods private. Otherwise, the changes are ok (invalidate is being called ~500k times, so lambda reduction is good).

Done in revision 15391.

#35 Updated by Stefanel Pezamosca almost 2 years ago

I have seen another place I could improve in ScrollableResults.get(boolean) method.

Committed in 9019a revision 15392.

#36 Updated by Constantin Asofiei almost 2 years ago

I think we can do the same for ScrollableResults.next(), and also avoid adding the 'impl' method - there is no reason for it. Just enclose the code in a try/catch.

next and get together have ~650k calls.

After this, lets consider 9019a for review and merge, and will continue in another branch as needed.

#37 Updated by Stefanel Pezamosca almost 2 years ago

  • % Done changed from 90 to 100

I think we can do the same for ScrollableResults.next(), and also avoid adding the 'impl' method - there is no reason for it. Just enclose the code in a try/catch.

Done in 9019a revision 15393.

#38 Updated by Constantin Asofiei almost 2 years ago

Thanks. Any other testing needed? We need to put this in trunk. I've previously tested ETF and was OK.

#39 Updated by Stefanel Pezamosca almost 2 years ago

Constantin Asofiei wrote:

Thanks. Any other testing needed? We need to put this in trunk. I've previously tested ETF and was OK.

I also tested the unittests and other application and they where ok.

#40 Updated by Constantin Asofiei almost 2 years ago

OK, will be merged after 8593e.

#41 Updated by Constantin Asofiei almost 2 years ago

  • Status changed from Review to Merge Pending

#42 Updated by Constantin Asofiei almost 2 years ago

Please merge 9019a now.

#43 Updated by Stefanel Pezamosca almost 2 years ago

  • Status changed from Merge Pending to Test

Branch 9019a was merged to trunk rev 15380 and archived.

Also available in: Atom PDF