Skip to content

Conversation

@SemMulder
Copy link

The SizeEstimator keeps a cache of ClassInfos but this cache uses Class objects as keys.
Which results in strong references to the Class objects. If these classes are dynamically created
this prevents the corresponding ClassLoader from being GCed. Leading to PermGen exhaustion.

We use a Map with WeakKeys to prevent this issue.

The SizeEstimator keeps a cache of ClassInfos but this cache uses Class objects as keys.
Which results in strong references to the Class objects. If these classes are dynamically created
this prevents the corresponding ClassLoader from being GCed. Leading to PermGen exhaustion.

We use a Map with WeakKeys to prevent this issue.
Copy link
Member

Choose a reason for hiding this comment

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

Rather than involve Guava, this can just be a WeakHashMap? it has weak keys.

Copy link
Author

Choose a reason for hiding this comment

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

A WeakHashMap would need to be synchronized and we would therefore lose concurrency.
I don't know if the SizeEstimator is used concurrently and hence felt it was safer to keep the concurrency of the original code despite the added complexity of a Guava Map.

Is it safe to drop the concurrency requirement?

EDIT: Also see this stackoverflow question

Copy link
Member

Choose a reason for hiding this comment

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

Good call, I see why you chose this. I agree with it and LGTM.

@SparkQA
Copy link

SparkQA commented Oct 24, 2015

Test build #1951 has finished for PR 9244 at commit 22a02a2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Oct 26, 2015

If there are no objections, I'll merge to master. This looks like a good targeted fix.

@asfgit asfgit closed this in feb8d6a Oct 27, 2015
@srowen
Copy link
Member

srowen commented Oct 27, 2015

merged to master

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.

3 participants