RFR: 8315559: Delay TempSymbol cleanup to avoid symbol table churn [v11]

Oli Gillespie ogillespie at openjdk.org
Mon Nov 27 18:09:04 UTC 2023


> Attempt to fix regressions in class-loading performance caused by fixing a symbol table leak in [JDK-8313678](https://bugs.openjdk.org/browse/JDK-8313678).
> 
> See lengthy discussion in https://bugs.openjdk.org/browse/JDK-8315559 for more background. In short, the leak was providing an accidental cache for temporary symbols, allowing reuse.
> 
> This change keeps new temporary symbols alive in a queue for a short time, allowing them to be re-used by subsequent operations. For example, when attempting to load a class we may call JVM_FindLoadedClass for multiple classloaders back-to-back, and all of them will create a TempNewSymbol for the same string. At present, each call will leave a dead symbol in the table and create a new one. Dead symbols add cleanup and lookup overhead, and insertion is also costly. With this change, the symbol from the first invocation will live for some time after it is used, and subsequent callers can find the symbol alive in the table - avoiding the extra work.
> 
> The queue is bounded, and when full new entries displace the oldest entry. This means symbols are held for the time it takes for 100 new temp symbols to be created. 100 is chosen arbitrarily - the tradeoff is memory usage versus 'cache' hit rate.
> 
> When concurrent symbol table cleanup runs, it also drains the queue.
> 
> In my testing, this brings Dacapo pmd performance back to where it was before the leak was fixed.
> 
> Thanks @shipilev , @coleenp and @MDBijman for helping with this fix.

Oli Gillespie has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:

 - Implement suggestions
 - Merge remote-tracking branch 'origin/master' into keepalive2
 - Fix indentation, rename test helper
 - Remove trailing whitespace
 - Remove is_enabled check, use modulo shortcut, add drain test
 - Set queue size to power of 2, use constant in test
 - Add missing atomic.hpp include
 - Switch to a ringbuffer instead of NonblockingQueue
 - Adress comments
   
   Fix indentation
   Improve tests
   Improve comment
   Remove redundant null check
   Improve naming
   Pop when >, not >= max len
 - Fix failing tests
   
   TempNewSymbol now increments refcount again, messing with the
   expectations. This is not a complete fix, I will have to read the
   individual cases and make sure they are correct.
 - ... and 2 more: https://git.openjdk.org/jdk/compare/18602c9b...d83ea056

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/16398/files
  - new: https://git.openjdk.org/jdk/pull/16398/files/6e06f007..d83ea056

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16398&range=10
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16398&range=09-10

  Stats: 861164 lines in 6978 files changed: 207725 ins; 528860 del; 124579 mod
  Patch: https://git.openjdk.org/jdk/pull/16398.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16398/head:pull/16398

PR: https://git.openjdk.org/jdk/pull/16398


More information about the hotspot-dev mailing list