JDK-8185034 : Cleanup and consolidate Metaspace coding
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 10
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2017-07-21
  • Updated: 2019-06-20
  • Resolved: 2018-05-07
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 11
11 b13Fixed
Related Reports
Duplicate :  
JDK-8190864 - Clean up overlapping functionality for printing/dumping statistics, etc. in metaspace
Relates :  
JDK-8198760 - Move global lock SpaceManager::_expand_lock to MutexLocker.cpp
Relates :  
JDK-8199430 - Rename MetaspaceAux to something more meaningful
Relates :  
JDK-8201572 - Improve Metaspace reporting
Relates :  
JDK-8199431 - Split up Metaspace class into a static and a non-static part
Relates :  
JDK-8176808 - Split up metaspace.cpp
Description
Metaspace coding has grown and would benefit from a bit of code grooming.

Note: out of scope for this enhancement is regrouping code to different files - see JDK-8176808.

- ChunkManager::verify_free_chunks_total() and ChunkManager::verify_free_chunks_count() are not used anywhere and could be removed
- ChunkManager::print_on is unused and seems only half implemented.
- ~SpaceManager: do not print the same stuff twice
- ChunkManager::locked_print_free_chunks() and ChunkManager::locked_print_sum_free_chunks() print exactly the same. locked_print_sum_free_chunks() gets its numbers by adding all (four) freelist statistics, whereas locked_print_free_chunks() just uses the running total in ChunkManager. Both are printed when a SpaceManager dies, so this leads to a repeated output
- Consolidate the different versions of MetaspaceAux::print_on(), MetaspaceAux::dump() and MetaspaceAux::print_metadata_for_nmt()
   (DONE as part of JDK-8201572)
- Consolidate MetaspaceAux::print_waste, MetaspaceAux::print_class_waste
   (DONE as part of JDK-8201572)
- More const correctness would be nice.
- rename MetaspaceAux::free_bytes() to something remotely useful, e.g. MetaspaceAux::committed_but_not_yet_allocated_bytes()
   (DONE as part of JDK-8201572)
- rename MetaspaceAux to MetaspaceUtils
   (DONE, JDK-8199430)
- split the Metaspace class into two classes. One class (ClassLoaderMetaspace) holds the metaspace for one classloader. The other class receives the static helper functions which are now part of class Metaspace. Potentially move parts of them to MetaspaceUtils.
   (DONE, JDK-8199431)
Comments
When I worked on metaspace report, I feel that MetaspaceAux introduced running sums to replace xxx_byte_slow, not sure they are still useful other than verification (?) Metaspace reporting walks CLDG at a safepoint to avoid safety issue.
28-02-2018
About splitting up metaspace, I still have this open: https://bugs.openjdk.java.net/browse/JDK-8176808. Splitting up metaspace.cpp is an extension in scope of 8176808, so feel free to re-purpose that issue. I agree, walking the CLDG should be avoided, if possible. One needs to do it for detailed analyses like the one Zhengyu does in his NMT printout of the class loaders. But not for sums. Also, I am not sure that we always do the correct locking when walking the CLDG. I'll take a look at what NMT does.
28-02-2018
We probably do not do the right locking when walking the CLDG and we don't properly apply read barriers so that CLDs don't go away while we are walking the CLDG.
28-02-2018
Yes, I really think we should open other another RFE for splitting the ChunkManager, SpaceManager and other Utilities etc out of Metaspace. It has too many classes that do similar things wrt to counters, which is why I could never keep the statistics that they kept straight. Some classes accumulate counts and some walk the CLDG for it. I think Metaspace should try to avoid walking the CLDG if at all possible. The only walking it should do is for the VirtualSpaceList. Anyway, I think Metaspace has grown too far beyond recognition. I think splitting it up would help you figure out what to log actually since the classes would be more self contained. A lot of this reminds me of NMT statistics collection which I think is the model you want to use. NMT accumulates counts. I've added Zhengyu to watch this RFE.
27-02-2018
I like "ClassLoaderMetaSpace" for Metaspace as held by the class loader. I also like MetaspaceUtils as a replacement for MetaspaceAux. About separating ChunkManager from metaspace.cpp - I am hesitant. At various times last year I tried to split metaspace.cpp into parts and each time it quickly got out of hand. I think it would make a lot of sense, but it is like pulling a thread which gets longer and longer, and there is only a limited amount of time I can spend on cleanups. So I would like to defer splitting up metaspace.cpp to a different issue. -- A different thing I struggle to find a good form for: I am currently looking at the many different "xx_bytes_slow" functions in MetaspaceAux and Metaspace (which is only the outward facing front, it continues in SpaceManager etc). So, a number of function which return one value, typically a sum of something, and to get that it walks the ClassLoaderDataGraph (for live metaspaces) or walks chunk lists (in the ChunkManager and the SpaceManager). I wonder whether it would make sense to replace them with a function which gets all values at once and collects it in an output structure. And maybe also separates printing from retrieving those values, so that only the retrieval happens under lock protection. An example of this is in ChunkManager, see the ChunkManagerStatistics and its functions I introduced sometime last year. But maybe I am overthinking, what do you think?
27-02-2018
Yes, I've found this hard to describe also. The ClassLoaderData points to a set of chunks of Metaspace for its classes. Maybe this should be named ClassLoaderMetaspace. Or something like MetaspaceChunkList. I wanted to rename MetaspaceAux to MetaspaceCounters but that name was already taken. I think MetaspaceAux is MetaspaceUtilities or Util would make more sense although it's not much different than Aux. Metaspace is a useful name of the class to hold the entire concept. Another thing I've thought is that we should move ChunkManager to metachunk.* and separate those concepts. Or put ChunkManager in its own file(s) and use namespace metaspace around the whole lot. And SpaceManager and VirtualSpaceManager should be moved out too.
27-02-2018
One thing which makes the code difficult to read is that "Metaspace" means two things: in general, the whole thing. And in particular, one object which holds the memory for a single class loader. So I think it would make sense to split the Metaspace class: - a class holding the metaspace memory for a single classloader. Maybe rename it? "MetaspaceHolder" or "SingleMetaspace" or similar. - a class with all the static usability function which do not directly operate on a single metaspace. These functions could be merged with MetaspaceAux, which is already a sink for all kind of static usability functions.
27-02-2018
@coleenp: > Please rename MetaspaceAux to something meaningful! Do you have a suggestion?
27-02-2018
Hope to do this for 11.
22-02-2018
[~stuefe], are you working on this issue for jdk 11?
18-12-2017
Please rename MetaspaceAux to something meaningful!
21-07-2017