Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PAYARA-3267 Fixing memory-wastage with JavaEETransactionImpl #3398

Merged
merged 9 commits into from
Nov 15, 2018

Conversation

svendiedrichsen
Copy link
Contributor

@svendiedrichsen svendiedrichsen commented Nov 8, 2018

Replacing usage of java.util.Timer/TimerTask with PayaraExecutorService/ScheduledExecutorService which is configured to remove cancelled tasks immediately.

This should fix #3396

…vice which is configured to remove cancelled tasks immediately
@arjantijms
Copy link
Contributor

Jenkins test please

@arjantijms arjantijms added PR: CLA CLA submitted on PR by the contributor community-contribution labels Nov 10, 2018
TimeUnit.valueOf(payaraExecutorServiceConfiguration.getThreadPoolExecutorKeepAliveTimeUnit()),
new SynchronousQueue<>(), (Runnable r) -> new Thread(r, "payara-executor-service-task"));
}

scheduledThreadPoolExecutor = new ScheduledThreadPoolExecutor(
Integer.valueOf(payaraExecutorServiceConfiguration.getScheduledThreadPoolExecutorCorePoolSize()),
(Runnable r) -> new Thread(r, "payara-executor-service-scheduled-task"));
scheduledThreadPoolExecutor.setRemoveOnCancelPolicy(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be configurable on the service

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think the old purge on x number of tasks should be also retained in case there is a performance hit on removing the tasks for many small and rapid transactions like in high volume web applications with timeouts enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed config from PayaraExecutorService

@@ -109,11 +112,13 @@
@Inject protected InvocationManager invMgr;

@Inject private Provider<RequestTracingService> requestTracing;


@Inject private PayaraExecutorService payaraExecutorService;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Payara Executor Service should be used for this. This is core application processing logic and I feel this could overload the Payara Executor Service which was planned for more internal tasks. For example if you have a web application performing 100s of requests per second with timeouts enabled we could be adding and removing 100s of timeout tasks per second on this Executor Pool. I approve of replacing timer with the more modern executor service though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced PayaraExecutorService with newly created ScheduledThreadPoolExecuter

…edThreadPoolExecutorService. Using this service also for local monitoring timer.
…enting usage of configuration of tx service property purge-cancelled-transactions-after. using scheduled task to purge.
@svendiedrichsen
Copy link
Contributor Author

@smillidge Implemented your requested changes. Maybe the production domain configuration Payara comes with should set a sensible default value for the transaction service property purge-cancelled-transactions-after to lower memory usage.

@svendiedrichsen
Copy link
Contributor Author

@smillidge Nice to see you going all-in on the PayaraExecutorService. Using it everywhere now. ;)

@arjantijms
Copy link
Contributor

Jenkins test please

… Setting purge property in production domain.xml to 500.
@lprimak
Copy link
Contributor

lprimak commented Nov 13, 2018

jenkins test

…-waste-using-timer

# Conflicts:
#	appserver/admin/production_domain_template/src/main/resources/config/domain.xml
@svendiedrichsen
Copy link
Contributor Author

Resolved merge conflicts

@arjantijms
Copy link
Contributor

Jenkins test please

@Pandrex247 Pandrex247 dismissed smillidge’s stale review November 15, 2018 15:32

Outdated and Resolved

@Pandrex247 Pandrex247 changed the title Fixing memory-wastage with JavaEETransactionImpl PAYARA-3267 Fixing memory-wastage with JavaEETransactionImpl Nov 15, 2018
@Pandrex247
Copy link
Member

Fixes #3396

@arjantijms arjantijms merged commit 9698ea0 into payara:master Nov 15, 2018
@svendiedrichsen svendiedrichsen deleted the memory-waste-using-timer branch November 15, 2018 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory wastage - JavaEETransactionImpl
5 participants