Configure transaction timeouts via properties #17357

marcelhanser · 2021-05-19T13:00:50Z

Add caching of the Transaction-Timeout config values

gsmet

Thanks.

So this needs two things:

a rebase so that history is clean
a proper assessment of the performance consequences of this change

/cc @Sanne

gsmet · 2021-05-19T13:20:27Z

.../src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java

@@ -104,18 +111,21 @@ protected Object invokeInOurTx(InvocationContext ic, TransactionManager tm) thro
    protected Object invokeInOurTx(InvocationContext ic, TransactionManager tm, RunnableWithException afterEndTransaction)
            throws Exception {

-        TransactionConfiguration configAnnotation = getTransactionConfiguration(ic);
+        Integer timeoutConfiguredForMethod = cache.computeIfAbsent(ic.getMethod(),
+                (m) -> extractTransactionConfigurationTimeoutFromAnnotation(ic));


We try to avoid lambdas in runtime code as they consume memory.

Also given it's a sensitive call site, better do a get() and return it if not null before doing the computeIfAbsent() (this would need to be moved to a method).

Now, I'm not exactly sure what cost this will have even when fully optimized.

@gsmet computeIfAbsent is right here, if you do a get first then compute the missing value you exposes yourself on concurrency issues.

The lambda can be an issue here as it's a capturing lambda and those have memory consideration, but I'm not against them (I know people on the community don't like them) it has a very slight impact with respect with transaction management ;)

I'm not saying computeIfAbsent shouldn't be used. I'm saying you should do a get before and return directly if not null. You do the computeIfAbsent after that. It's a CHM, you won't have concurrency issues doing that. Locking is less efficient when using computeIfAbsent so if you can avoid calling it, it's a win. And here, at some point, all the values will be in the cache so, after the warmup, things will be significantly better because you won't call computeIfAbsent() at all. @Sanne was the one mentioning this to me first and I did numerous HV benchmarks proving this point (at least with the JDK used at that time).

As for lambdas, we decided as a community to avoid them as much as possible in runtime code (i.e. except if they really bring something to the plate. This is not the case here.

After giving it some additional thoughts, I think the cache should be only used for methods annotated with timeoutFromConfigProperty. It won't cost that much to not use the cache in other cases and it will avoid the cost of it entirely. My wild guess is that having the cache will have a higher cost if either we don't have anything set or if we are just using timeout.

@marcelhanser and you thought it would be easy ;)

I thought i do a small little pull-request and go to get a beer - buuuut^^
hmm... How do we know if a method is annotated with timeoutFromConfigProperty, without looking into it and do the costly reflection call? That was the reason for me to cache 'all'

@gsmet oh yeah, I remember already having this discussion, sorry not to have remembered it ;)

So yes, making a get() before is good, there is a slight risk to compute two times the cache entry but it should not be an issue here as computing it is not very ressource consuming.

After giving it some additional thoughts, I think the cache should be only used for methods annotated with timeoutFromConfigProperty.

Retrieving annotation is costly I think, it's reflection so it may be cached, but still ... I remember we discuss it with @Sanne and agree that at some point we will cache the TransactionConfiguration to avoid too much reflection.
So the correct thing to cache is not the timeout but the full TransactionConfiguration object to avoid the reflective all.

Hum, forget what I just told, caching the annotation will not be usefull here as we want to avoid the call to the config ...

So I'll keep it as is, or create a TransactionConfigurationInfo object that will have the informations resolved from the annotation, as the annotation currently only deals with timeout it may not be necessary.

So yes, making a get() before is good, there is a slight risk to compute two times the cache entry but it should not be an issue here as computing it is not very ressource consuming.

There isn't a race: first you try get(), when it fails you still do the computeIfAbsent so you'll have a lock. Worst thing that can happen is that the initial call does an additional pointless get() ... totally ok.

Sanne · 2021-05-19T13:27:23Z

It's a good idea, but let me ask: what do you expect will happen if an annotated method is invoked while a Transaction is already started? How will multiple such annotations compose?

I think this should be documented clearly.

loicmathieu · 2021-05-19T13:34:14Z

.../src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java

@@ -104,18 +111,21 @@ protected Object invokeInOurTx(InvocationContext ic, TransactionManager tm) thro
    protected Object invokeInOurTx(InvocationContext ic, TransactionManager tm, RunnableWithException afterEndTransaction)
            throws Exception {

-        TransactionConfiguration configAnnotation = getTransactionConfiguration(ic);
+        Integer timeoutConfiguredForMethod = cache.computeIfAbsent(ic.getMethod(),
+                (m) -> extractTransactionConfigurationTimeoutFromAnnotation(ic));


@gsmet computeIfAbsent is right here, if you do a get first then compute the missing value you exposes yourself on concurrency issues.

The lambda can be an issue here as it's a capturing lambda and those have memory consideration, but I'm not against them (I know people on the community don't like them) it has a very slight impact with respect with transaction management ;)

loicmathieu · 2021-05-19T13:34:57Z

.../src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java

@@ -34,6 +40,8 @@
 public abstract class TransactionalInterceptorBase implements Serializable {

    private static final long serialVersionUID = 1L;
+    private static final Logger log = Logger.getLogger(TransactionalInterceptorBase.class);
+    private final Map<Method, Integer> cache = new ConcurrentHashMap<>();


Maybe make it static

I'm not sure using Method as cache key is a good idea here, the method object is not very lightweigth and we may retain it for a long time.

My experience with HV is: caching things at a method level costs a lot. Because you either use Method or generate a full signature and in both cases, it's not fast.

As for making it static, depends. If you make it static, you will need to be able to clean it properly at shutdown.

.../src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java

loicmathieu · 2021-05-19T13:45:38Z

@Sanne we enforce using this transaction at the entry level of the transaction (I think you alreay ask for it when I first create the annotation ;) ), see

quarkus/extensions/narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/interceptor/TransactionalInterceptorBase.java

Line 294 in 863f49d

private void checkConfiguration(InvocationContext ic) {

marcelhanser · 2021-05-20T06:53:14Z

@gsmet I rebased and squashed my stuff together.
I also adressed the other comments as far as i could. Is there anything left i can do:)?
Btw: I tried to write some tests. But it seems not to be simple to get the timout for the current transaction without doing "ugly" things - Would it be ok for you todo "ugly" things in test code? And if so in which module should i write them?
Another way would be to simulate a "long" running task in the transaction... hmm ill try something and place it in the integration-test/jpa-h2 :)

marcelhanser · 2021-06-11T10:56:05Z

@Sanne @loicmathieu @gsmet
Hello everyone. I would like to somehow get that issue solved. Can i still do something to assist you? Or do i just need to wait?
Best & Thx
Marcel

gsmet · 2021-06-11T11:23:53Z

I'll have a look later today. IIRC, there are still things that need fixing.

Resolves: quarkusio#15752

gsmet · 2021-06-11T18:06:00Z

@marcelhanser I rebased and added a new commit. The main idea is to avoid paying any cost in the normal case. I think I kept all the behaviors you implemented (thanks for the tests!). Could you have a look?

marcelhanser · 2021-06-15T06:10:46Z

Awesome - thx @gsmet - looks good to me.

gsmet · 2021-06-28T09:09:05Z

@Sanne could you have a second look at this one given I pushed an additional commit that changes quite a lot of things? Thanks!

Sanne · 2021-06-28T10:20:05Z

thanks @gsmet - looks nice, but help me understand why isn't the annotation lookup included in the cache as well?

I would probably have cached the whole invocation into getTransactionTimeoutFromAnnotation.

Sanne

As mentioned in other comments: I'm not sure why the cache isn't caching all operations from getTransactionTimeoutFromAnnotation , but it's not a blocker.

gsmet · 2021-06-28T11:38:44Z

@Sanne I did that on purpose. My wild guess is that the annotation lookup will take less time than the cache lookup based on a Method key. That might be an incorrect assumption but that's my experience with HV tells me to do.

I'm not entirely sure as the annotation lookup does 2 map lookups based on Class.

I don't know if you have an easy way to check if my assumption is correct or not?

Sanne · 2021-06-28T14:37:41Z

@gsmet ok that sounds reasonable, thanks for explaining. I won't check it: IMO it's totally OK to use your experience in this case.

Sanne · 2021-07-13T08:44:01Z

To clarify my above comments: this PR is good for me. But it wasn't merged as it's in status "changes requested" by @gsmet . Have they been addressed?

gsmet · 2021-07-13T14:44:01Z

Merging, thanks everyone!

quarkus-bot bot added area/hibernate-orm Hibernate ORM area/narayana Transactions / Narayana area/persistence OBSOLETE, DO NOT USE labels May 19, 2021

marcelhanser mentioned this pull request May 19, 2021

Configuration transactions geoand/quarkus#6

Open

geoand mentioned this pull request May 19, 2021

Allow setting transaction timeout from a config property #15832

Closed

geoand requested a review from loicmathieu May 19, 2021 13:08

gsmet requested changes May 19, 2021

View reviewed changes

gsmet requested a review from Sanne May 19, 2021 13:21

gsmet changed the title ~~Configuration transactions~~ Configure transaction timeouts via properties May 19, 2021

loicmathieu reviewed May 19, 2021

View reviewed changes

marcelhanser force-pushed the configuration-transactions branch 2 times, most recently from 4c41c6c to ffa831e Compare May 20, 2021 06:23

geoand and others added 4 commits June 11, 2021 18:55

Allow setting transaction timeout from a config property

33690e0

Resolves: quarkusio#15752

Add method based caching for property based timout configuration

6b8e02e

Resolves: quarkusio#15752

Add tests for transaction configuration

b450e8b

Resolves: quarkusio#15752

Avoid paying the cost of CHM access for standard cases

bbd8f25

gsmet force-pushed the configuration-transactions branch from e727d9d to bbd8f25 Compare June 11, 2021 18:05

Sanne approved these changes Jun 28, 2021

View reviewed changes

Sanne requested a review from gsmet June 28, 2021 14:37

gsmet approved these changes Jul 13, 2021

View reviewed changes

gsmet merged commit 29c2216 into quarkusio:main Jul 13, 2021

quarkus-bot bot added this to the 2.1 - main milestone Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure transaction timeouts via properties #17357

Configure transaction timeouts via properties #17357

marcelhanser commented May 19, 2021

gsmet left a comment

gsmet May 19, 2021

loicmathieu May 19, 2021

gsmet May 19, 2021

gsmet May 19, 2021

marcelhanser May 19, 2021

loicmathieu May 19, 2021

loicmathieu May 19, 2021

Sanne May 19, 2021

Sanne commented May 19, 2021

loicmathieu May 19, 2021

loicmathieu May 19, 2021

loicmathieu May 19, 2021

gsmet May 19, 2021

loicmathieu commented May 19, 2021

marcelhanser commented May 20, 2021 •

edited

Loading

marcelhanser commented Jun 11, 2021

gsmet commented Jun 11, 2021

gsmet commented Jun 11, 2021

marcelhanser commented Jun 15, 2021 •

edited

Loading

gsmet commented Jun 28, 2021

Sanne commented Jun 28, 2021

Sanne left a comment

gsmet commented Jun 28, 2021

Sanne commented Jun 28, 2021

Sanne commented Jul 13, 2021

gsmet commented Jul 13, 2021

Configure transaction timeouts via properties #17357

Configure transaction timeouts via properties #17357

Conversation

marcelhanser commented May 19, 2021

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanne commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicmathieu commented May 19, 2021

marcelhanser commented May 20, 2021 • edited Loading

marcelhanser commented Jun 11, 2021

gsmet commented Jun 11, 2021

gsmet commented Jun 11, 2021

marcelhanser commented Jun 15, 2021 • edited Loading

gsmet commented Jun 28, 2021

Sanne commented Jun 28, 2021

Sanne left a comment

Choose a reason for hiding this comment

gsmet commented Jun 28, 2021

Sanne commented Jun 28, 2021

Sanne commented Jul 13, 2021

gsmet commented Jul 13, 2021

marcelhanser commented May 20, 2021 •

edited

Loading

marcelhanser commented Jun 15, 2021 •

edited

Loading