On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga <d...@openjdk.org> wrote:
>> ### Summary >> This PR just replaces `ThreadCritical` with a lock specific to NMT. >> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT. >> I've implemented the new lock with a semaphore so that it can be used early >> before VM init. There is also the possibility of adding assertions in >> places we expect NMT to have synchronization. I haven't added assertions yet >> in many places because some code paths such as the (NMT tests) don't lock >> yet. I think it makes sense to close any gaps in locking in another PR in >> which I can also add more assertions. >> >> Testing: >> - hotspot_nmt >> - gtest:VirtualSpace >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with two > additional commits since the last revision: > > - add a comment explaining lock rank > - remove unnecessary dropping of tracking level The easiest action to take would be to remove the `ThreadCritical` that attempts to keep NMT accounting in-sync in arena.cpp. It doesn't actually cover all cases, so we might as well not bother locking at all for slightly better performance and reduce the risk of bad lock ordering. The next best solution would be to make arena size a subset of malloc size, and make mtChunk only responsible for free chunks not yet associated with other NMT categories. Then there would be no need for special adjustments when reporting, and thus no need for the special `ThreadCritical` locking here. And when calculating total sizes, we can simplify the calculation to summing up all "malloc" in MemoryCounter. No need to make a distinction between when it is/isn't appropriate to add on arena sizes. This would be a breaking change since it changes the meaning of arena, malloc, and mtChunk. The third and highest effort solution would be to remove the chunk pooling altogether as suggested in https://bugs.openjdk.org/browse/JDK-8325890. This shares the benefits of solution 2, but also requires performance testing and has consequences that extend beyond NMT. >Also, the current locking does not guarantee correctness anyway. There are >several places in the coding where we calculate e.g. the sum of all mallocs, >but malloc counters are modified (rightly so) out of lock protection. Yes, still none of the three above solutions solve this. Even with solution 2 or 3 it would be possible to update the arena sizes for each category concurrently with reporting (unless we introduce new locking). ------------- PR Comment: https://git.openjdk.org/jdk/pull/20852#issuecomment-2450515494
