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.

Sign up for GitHub

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

Jump to bottom

Correct work with weak references in QNameCache #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Correct work with weak references in QNameCache #37

wants to merge 1 commit into from

Conversation

skel-nl
Copy link

@skel-nl skel-nl commented Nov 29, 2017

Quote from WeakHashMap's javadoc:

The value objects in a WeakHashMap are held by ordinary strong references. Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded.

So the current code leads to memory leaks in case of big amount different qnames because it's name (key) is used in QName. My experience - about 1 GB of uncollected garbage for 2 days in production.

This behaviour is reproduced in https://gist.github.com/skel-nl/291abe36aed9a90f9313f9129b4fbd10
Example with fix like in this pull request - https://gist.github.com/skel-nl/1768180608ed7a24f8f28c8de8792ff8

@codecov-io
Copy link

codecov-io commented Nov 29, 2017 •
edited
Loading

Codecov Report

Merging #37 into master will increase coverage by 0.05%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #37      +/-   ##
============================================
+ Coverage     39.51%   39.57%   +0.05%     
- Complexity     1475     1479       +4     
============================================
  Files           145      145              
  Lines          9441     9455      +14     
  Branches       1182     1189       +7     
============================================
+ Hits           3731     3742      +11     
- Misses         5338     5339       +1     
- Partials        372      374       +2
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/dom4j/tree/QNameCache.java 76.62% <76.92%> (-1.16%) 21 <6> (+4)
src/main/java/org/dom4j/tree/NamespaceCache.java 95.34% <0%> (+2.32%) 14% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b14152...0e4c9c9. Read the comment docs.

This was referenced Nov 29, 2017
Closed
Closed
@skel-nl
Copy link
Author

skel-nl commented Nov 29, 2017

Also I don't completely understand why this cache is necessary.

@FilipJirsak FilipJirsak self-assigned this Jul 1, 2018
@FilipJirsak FilipJirsak added this to the 3.0.0 milestone Jul 1, 2018
@FilipJirsak
Copy link
Contributor

QNameCache is used to reduce the number of QName instances created - it is assumed that only a limited set of QNames will be used. If you have a lot of different QNames in your case, you do not need to use the cache and you can create QName instances using the constructor.
QNameCache has performance problems in a multi-threaded environment and should be replaced in future versions.

@el-dot
Copy link

el-dot commented Mar 15, 2019 •
edited
Loading

you do not need to use the cache and you can create QName instances using the constructor.

It is not an option in complicated system highly dependent on dom4j.

Also similar leak happens in NamespaceCache, where prefixes and namespace names are hard references and WeakReferences are not removed from maps after garbage collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@FilipJirsak FilipJirsak

Labels
enhancement
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@skel-nl @codecov-io @FilipJirsak @el-dot