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

Replaced unused WeakReference with Boolean in Memory.allocatedMemory #1233

Merged

Conversation

joerg1985
Copy link
Contributor

The Memory.allocatedMemory Map had a unused WeakReference as value,
only the key is used to work with the Memory instance. Replacing the
value will reduce the overhead for each Memory instance.

The Memory.allocatedMemory Map had a unused WeakReference as value,
only the key is used to work with the Memory instance. Replacing the
value will reduce the overhead for each Memory instance.
@dbwiddis
Copy link
Contributor

I'm trying to figure out what was originally intended here. I originally asked "why Boolean", which seems to be the standard pattern everywhere, using a constant TRUE to avoid object creation overhead, as you've done. That apparently is an advantage over storing null because you could use get() to determine the presence of a value rather than containsKey(). Although we do neither. We make a copy of the keyset and iterate it, with the copy creating a strong reference in the process (so we don't try to call dispose() on null).

So I have no objection to it as implemented but I still don't know why null can't be the value here.

Letting someone else handle this, as I don't want to touch that map implementation....

@joerg1985
Copy link
Contributor Author

You are right, it is a common way to use an Boolean, e.g. Collections.newSetFromMap.
Apart from this the signature of the map could be changed to Map<Memory, Void> and the value could be set to null. I do not have a prefered solution, so i could change this if it helps to get this merged?
Thank you for the review and the comment to this pr!

@dbwiddis
Copy link
Contributor

I think the performance difference in Void vs. a constant object would trivial, and am content with the Boolean pattern now that I fully understand its purpose in a synchronized environment. (Even if we don't use get() now we could in the future so no need to break the pattern.)

I am leaving this for @matthiasblaesing to handle as he's done more research into #1211 and may have a better idea whether this fits into another solution he may be contemplating. Looks good to me, though.

@matthiasblaesing
Copy link
Member

Looks sane to me. Discussing memory usage in this case is a bit of black magic. Reference objects are by definition special and might also be treated specially by the VM and thus optimized (assuming WeakHashMap is not implemented on the VM level, it will also create a WeakReference). The code looks cleaner and might improve Heap utilization.

It should be noted, that optimisations on memory come at a price: You are removing heap pressure and thus GC might delayed, but it is also stupid to blow up memory just to let the GC be more agressive.

@matthiasblaesing matthiasblaesing merged commit 3d9b024 into java-native-access:master Jul 20, 2020
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