-
-
Notifications
You must be signed in to change notification settings - Fork 55.7k
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
Added finalize back in #4029
Added finalize back in #4029
Conversation
Instead of merging this one I suggest reverting #4014. |
agree. reverting #4014 is also more correct in terms of git practices, we can be sure then that we get back exactly to the previous state. |
Replacing with #4031 and closing. |
This removed finalize and left n_delete in release(). Instead of doing random reverts, how about I fix them properly and submit a pull request? |
@sgjava , the solution that was implemented before your changes allows both manual memory clean-up via |
If you really know a better solution than the existing one - let's discuss it in details before merging changes into almost released code. |
finalize() is not traditional and is in fact bad practice as I've pointed out several times. It can behave differently on different JVM implementations and versions. In fact, it is not called at all a lot of times and causes the Finalizer queue to get backed up besides performance issues. If finalize were best practice it would be used for JDBC connections, file I/O. etc. to clean up resources. Even Oracle says not to use them http://docs.oracle.com/cd/E13188_01/jrockit/docs142/devapp/codeprac.html#998529 That being said I'd still like to make the changes I've made, but keep finalize since you guys want it there. Here's what I propose: o Keep finalize() Let me know if this sounds good and I can make the changes once all the reverts are done. |
Since created Java |
Yea, but the way of disposing both Java Object and native memory at the same time are flawed since it's based on finalize(). Native memory is the major concern. If you use finalize() then you are guaranteed that Object will be there possibly until JVM exit which could be days or weeks depending on the application. You may say this is only a few bytes, but why leave unused Objects in memory when they should have been GC? Any ways, I agreed with you to let release() only do Mat::release. It would still be nice to have public access to delete() because I'll just patch the source on my end to remove finalize() since I don't want to use a broken implementation. You have to remember there's no protection against calling release() twice. You will get *** Error in `/usr/lib/jvm/jdk1.8.0/bin/java': malloc(): memory corruption: 0x00007fd814230fb0 *** and crash the JVM. This should be protected by a check of nativeObj. |
Again: the |
Yea, I understand how release and delete works. I'm working on a page describing the memory issues using NetBeans profiler and Valgrind. There are very few corner cases that would require explicit GC (I've never used it in 20 years of Java development). "The reason everyone always says to avoid System.gc() is that it is a pretty good indicator of fundamentally broken code. Any code that depends on it for correctness is certainly broken; any that rely on it for performance are most likely broken." I believe that is the case here. http://stackoverflow.com/questions/2414105/why-is-it-bad-practice-to-call-system-gc |
Finalize added back in and nativeObj safety check in case delete() or release() already called.