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

1052: Memory leak in RestRequestCache #1167

Closed
wants to merge 4 commits into from

Conversation

@erikj79
Copy link
Member

@erikj79 erikj79 commented May 25, 2021

This patch adds a regularly scheduled cache eviction call to the RestRequestCache.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/skara pull/1167/head:pull/1167
$ git checkout pull/1167

Update a local copy of the PR:
$ git checkout pull/1167
$ git pull https://git.openjdk.java.net/skara pull/1167/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1167

View PR using the GUI difftool:
$ git pr show -t 1167

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/skara/pull/1167.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 25, 2021

👋 Welcome back erikj! A progress list of the required criteria for merging this PR into pr/1166 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title SKARA-1052 1052: Memory leak in RestRequestCache May 25, 2021
@openjdk openjdk bot added the rfr label May 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2021

Webrevs

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks fine, although I raised a question about the default frequency.

*/
Duration cacheEvictionInterval() {
if (!config.contains("runner") || !config.get("runner").contains("cache_eviction_interval")) {
var defaultValue = Duration.ofMinutes(5);
Copy link
Member

@kevinrushforth kevinrushforth May 25, 2021

Choose a reason for hiding this comment

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

Have you done any testing to see if a default interval of 5 minutes is the right frequency for cache evictions? Naively, it seems like it might be too often. Is there a performance impact?

Copy link
Member

@edvbld edvbld May 26, 2021

Choose a reason for hiding this comment

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

I agree with Kevin, I think we could probably keep at least 60 minutes of cache, if not even more. Or I might have missed something 🤷😄

Copy link
Member Author

@erikj79 erikj79 May 26, 2021

Choose a reason for hiding this comment

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

That is a good point. In my initial patch I used the scheduling frequency (which is what the watchdog uses) instead of this new configuration value, which is 10 seconds, and it worked fine on staging. I thought that was a bit excessive so I increased to a default of 5 mins, which matches the lowest "maxAge" in the cache.

While we could certainly live with a longer interval here, 24h would still solve our problems, I don't think we have much to gain from increasing it longer than my suggested 5 mins. The nature of the bots is that they run pretty much non stop, using a lot of cpu. I can't see how running this little method will have any noticeable impact on performance, even if the cache has thousands or even millions of valid entries. It shouldn't take more than milliseconds on one of 8-32 threads (depending on bot runner config).

If we start seeing problems, this value can always be changed in the future, or configured for any specific bot runner configuration.

Copy link
Member

@kevinrushforth kevinrushforth May 26, 2021

Choose a reason for hiding this comment

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

Seems OK to me.

Copy link
Member Author

@erikj79 erikj79 May 26, 2021

Choose a reason for hiding this comment

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

Also just to clarify as I realized there was a misunderstanding in offline discussion. This patch is not changing the functional behavior of the cache in any way. The entries that are being removed are just those older than "maxAge", which were already disqualified in the send method.

I have also verified the behavior in staging over night and the metrics show that we have the same amount of cache hits/min with the patch as before.

edvbld
edvbld approved these changes May 26, 2021
Copy link
Member

@edvbld edvbld left a comment

Looks good

*/
Duration cacheEvictionInterval() {
if (!config.contains("runner") || !config.get("runner").contains("cache_eviction_interval")) {
var defaultValue = Duration.ofMinutes(5);
Copy link
Member

@edvbld edvbld May 26, 2021

Choose a reason for hiding this comment

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

I agree with Kevin, I think we could probably keep at least 60 minutes of cache, if not even more. Or I might have missed something 🤷😄

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/1166 to master May 26, 2021
@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented May 26, 2021

The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout SKARA-1052-add-metrics
git fetch https://git.openjdk.java.net/skara master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

@openjdk openjdk bot commented May 26, 2021

@erikj79 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

🌎 Applicable reviewers for one or more changes in this pull request are spread across multiple different time zones. Please consider waiting with integrating this pull request until it has been out for review for at least 24 hours to give all reviewers a chance to review the pull request.

After integration, the commit message for the final commit will be:

1052: Memory leak in RestRequestCache

Reviewed-by: kcr, ehelin

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • d5e31d2: Add metrics for RestRequestCache

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 26, 2021
@erikj79
Copy link
Member Author

@erikj79 erikj79 commented May 26, 2021

/integrate

@openjdk openjdk bot closed this May 26, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 26, 2021

@erikj79 Since your change was applied there has been 1 commit pushed to the master branch:

  • d5e31d2: Add metrics for RestRequestCache

Your commit was automatically rebased without conflicts.

Pushed as commit ca8d1c3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants