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

[2.1] 1584256: Detach consumers from persistence context to avoid OOM - ENT-583 #2016

Merged
merged 2 commits into from
Jun 5, 2018

Conversation

awood
Copy link
Contributor

@awood awood commented May 31, 2018

Updating the status of a large number of consumers (roughly 10000) could
run the JVM out of memory due to all the consumers referenced in the
consumerStackedEnts map. During status calculation, the consumer
facts are accessed and during those accesses, the Hibernate proxy object
is replaced by the actual map of facts. The consumerStackedEnts map
would accordingly grow as each status was calculated.

This patch detaches several model objects as soon as we stop needing
them and most importantly, it detaches all the consumers in the
consumerStackedEnts map. The consumers are then fetched again, the
status is updated, and then immediately detached to keep the persistence
context at roughly a constant size.

Copy link
Contributor

@Ceiu Ceiu left a comment

Choose a reason for hiding this comment

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

Code itself looks fine; though I'm curious if there are other consequences of blasting the entire context and unlinking some entities. Specifically, though the processPoolUpdates method.

There's an extra bit of code that can be removed from deleteExpiredPoolsImpl that flushes (and intends to clear) which can be removed entirely due to the extra flushing in deletePools

// Recalculate status for affected consumers
for (List<String> subList : Lists.partition(consumers, 1000)) {
for (String consumerId : subList) {
Consumer consumer = consumerCurator.get(consumerId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we've blasted the Hibernate cache, this is hitting the DB for each consumer. I imagine we'd see some substantial gains if we bulked up the lookup for the entire sublist of consumers. Given our concerns with memory, this may mean dropping the size of the partitioning down from 1000 (and maybe converting that to a constant while we're at it), but any bulking at all will at least halve the number of consumer lookups we're executing.

@awood awood force-pushed the awood/1584256-oom-2.1 branch 3 times, most recently from 0d8e1e0 to 1e0ff6e Compare June 1, 2018 20:45
@awood
Copy link
Contributor Author

awood commented Jun 1, 2018

Before:
expired-pools-2 1-before-713s

  • 713s to run
  • perm generation maxed out at 5.2 GB

After:
expired-pools-2 1-after-612s

  • 612s to run
  • perm generation maxed out at 2GB

@awood awood removed the in progress label Jun 1, 2018
@awood
Copy link
Contributor Author

awood commented Jun 4, 2018

@candlepin-pull-request-bot retest this please

@awood awood changed the title [2.1] 1584256: Clear persistence context to avoid OOM [2.1] 1584256: Detach consumers from persistence context to avoid OOM Jun 4, 2018
@barnabycourt barnabycourt changed the title [2.1] 1584256: Detach consumers from persistence context to avoid OOM [2.1] 1584256: Detach consumers from persistence context to avoid OOM - ENT-583 Jun 4, 2018
Updating the status of a large number of consumers (roughly 10000) could
run the JVM out of memory due to all the consumers referenced in the
consumerStackedEnts map.  During status calculation, the consumer facts
are accessed and during those accesses, the Hibernate proxy object is
replaced by the actual map of facts.  The consumerStackedEnts map would
accordingly grow as each status was calculated.

This patch detaches several model objects as soon as we stop needing
them and most importantly, it detaches all the consumers in the
consumerStackedEnts map.  The consumers are then fetched again, receive
an updated status, and are then immediately detached to keep the
persistence context at roughly a constant size.
- Added a new method, getConsumers(Collection), to ConsumerCurator for
  performing bulk consumer fetching/lookup
- Updated the consumer status recalculation loop in CPM.deletePools to
  use the bulk consumer lookup
@awood awood merged commit 3ec9159 into candlepin-2.1-HOTFIX Jun 5, 2018
@Ceiu Ceiu deleted the awood/1584256-oom-2.1 branch July 30, 2018 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants