Remove all calls to System.gc() in PerQueryCPUMemAccountantFactory

[vrajat]

Calls to System.gc() force a GC pause and is not recommended. From experience such a call can worsen the state of the system rather than help.

A small unrelated changed to log if the accountant used a callback to cancel is also in this PR.

Closes #16373

[Copilot]

Pull Request Overview

This PR removes all calls to System.gc() from the PerQueryCPUMemAccountantFactory to improve system performance. Forced garbage collection calls can cause unnecessary GC pauses and may worsen system performance rather than help with memory management.

Key changes:

• Removed GC-related configuration properties and logic from QueryMonitorConfig
• Eliminated forced garbage collection calls in query monitoring and termination logic
• Simplified the query killing mechanism by removing GC backoff counting

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
QueryMonitorConfig.java	Removed GC-related configuration fields, constructors, and getter methods
PerQueryCPUMemAccountantFactory.java	Removed System.gc() calls and related logic from query monitoring and killing mechanisms

[yashmayya]

The new tests added in #16299 are failing.

[praveenc7]

I understand the intention of the change to improve performance. Have we validated through some integ-test or running in a prod like environment with actual queries that it doesn't cause any functionality regression.

Can we wait a till Monday before we merge? Let me check why we added forced GC. I think we did some analysis when adding this

[vrajat]

I understand the intention of the change to improve performance. Have we validated through some integ-test or running in a prod like environment with actual queries that it doesn't cause any functionality regression.

Can we wait a till Monday before we merge? Let me check why we added forced GC. I think we did some analysis when adding this

@praveenc7 - Would definitely like to understand why this was added. This call is consistently causing a major collection and pausing the JVM both in production and a cluster test based on production-sized dataset and workloads.

Java Docs also mention this is should be generally avoided: https://www.oracle.com/java/technologies/javase/gc-tuning-6.html#other_considerations

You can check the JFR screenshot in the attached issue #16373

[vrajat]

The new tests added in #16299 are failing.

Found a bug - fixed.

[vrajat]

This SO answer expresses what I know from experience well: https://stackoverflow.com/a/2414120

The call was added 3 years ago: 69ad81b

[yashmayya]

+1, invoking GC explicitly is an anti-pattern. It generally reduces overall GC throughput while consuming more resources, and the only likely scenario it could be useful in is to tune latency vs throughput at a particular point in the application lifecycle (which isn't applicable to Pinot and definitely not the way we're using it here). Furthermore, for garbage collectors like G1GC, explicit System.gc() calls trigger a full, stop-the-world (STW) garbage collection by default.

[yashmayya]

This SO answer expresses what I know from experience as well. https://stackoverflow.com/a/2414120

This answer in the same thread also explains how it can actually hurt performance and reduce overall GC throughput - https://stackoverflow.com/a/2414621.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.31%. Comparing base (1a476de) to head (b1bb489).
Report is 463 commits behind head on master.

Files with missing lines	Patch %	Lines
...re/accounting/PerQueryCPUMemAccountantFactory.java	57.14%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #16374      +/-   ##
============================================
+ Coverage     62.90%   63.31%   +0.41%     
+ Complexity     1386     1363      -23     
============================================
  Files          2867     2976     +109     
  Lines        163354   172766    +9412     
  Branches      24952    26459    +1507     
============================================
+ Hits         102755   109392    +6637     
- Misses        52847    55024    +2177     
- Partials       7752     8350     +598     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-11	63.27% <57.14%> (+0.40%)	⬆️
java-21	63.30% <57.14%> (+0.47%)	⬆️
skip-bytebuffers-false	?
skip-bytebuffers-true	?
temurin	63.31% <57.14%> (+0.41%)	⬆️
unittests	63.31% <57.14%> (+0.41%)	⬆️
unittests1	56.45% <57.14%> (+0.63%)	⬆️
unittests2	33.31% <0.00%> (-0.27%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.