[jira] [Work logged] (WW-5539) Concurrency performance enhancements

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/WW-5539?focusedWorklogId=1031535&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031535 ]

ASF GitHub Bot logged work on WW-5539:
--------------------------------------

                Author: ASF GitHub Bot
            Created on: 21/Jul/26 18:56
            Start Date: 21/Jul/26 18:56
    Worklog Time Spent: 10m 
      Work Description: lukaszlenart opened a new pull request, #1799:
URL: https://github.com/apache/struts/pull/1799

   Fixes [WW-5539](https://issues.apache.org/jira/browse/WW-5539)
   
   Three core classes had coarse locks compensating for caches that were not thread-safe. This makes the caches genuinely concurrent and deletes the locks.
   
   ## What was wrong
   
   **`StrutsTypeConverterHolder`** — a container singleton whose four `HashMap`/`HashSet` caches were read with no lock at all by `XWorkConverter.lookup()` while being written elsewhere. This is a real data race, not just contention. The new test reproduces it against the unfixed code: a lost registration, and a `HashMap$Node`→`HashMap$TreeNode` `ClassCastException` thrown from concurrent structural modification during resize.
   
   **`XWorkConverter`** — `getConverter()` held `synchronized (clazz)`. Locking on a `Class` object is a globally visible monitor that any unrelated library can also lock, and it serialised every conversion for a given action class, including pure cache hits. `registerConverter`/`registerConverterNotFound` were `synchronized` methods on the singleton, though they only ever excluded each other — the readers in `lookup()` were never covered.
   
   **`DefaultActionValidatorManager`** — `getValidators()` was `synchronized` on the singleton, so every validated request in the application serialised on it. The lock also covered the per-request `Validator` construction loop, which operates on per-request objects and never needed mutual exclusion.
   
   ## Approach
   
   Concurrent collections, then the locks come out. Where a computation is expensive and provably safe to guard, `computeIfAbsent`; where it is not, check-then-act with cheap idempotent duplicate work.
   
   `TypeConverterHolder` gains one `default` method, `computeMappingIfAbsent(Class, Function)`, so third-party implementations of this SPI keep working unchanged. `StrutsTypeConverterHolder` overrides it with a real `computeIfAbsent`, storing a sentinel for negative results so the builder runs exactly once per class whether or not the class has a mapping — the no-mapping case is the common one for ordinary actions, and the builder walks the whole class hierarchy doing classpath lookups and reflection.
   
   ## Measurements
   
   Throughput, 3s measured after 20k warm-up iterations, on the same machine, before vs after. Second of two runs each; both runs agreed closely.
   
   `DefaultActionValidatorManager.getValidators`:
   
   | threads | before (ops/s) | after (ops/s) | ratio |
   |--------:|---------------:|--------------:|------:|
   | 1       | 11,100         | 11,300        | 1.0x  |
   | 4       | 10,400         | 41,866        | 4.0x  |
   | 16      | 10,700         | 77,366        | 7.2x  |
   | 64      | 12,100         | 77,700        | 6.4x  |
   
   The before column is the signature of a global lock: flat at ~10k ops/s no matter how many threads you add. This is the headline change.
   
   `XWorkConverter.getConverter`:
   
   | threads | before (ops/s) | after (ops/s) | ratio |
   |--------:|---------------:|--------------:|------:|
   | 1       | 26,342,966     | 94,234,166    | 3.6x  |
   | 4       | 13,362,366     | 341,507,566   | 25.6x |
   | 16      | 12,643,300     | 850,345,433   | 67x   |
   | 64      | 12,257,333     | 847,833,400   | 69x   |
   
   Treat the converter *magnitudes* with suspicion — a warm cache hit is a single map lookup whose result the harness discards, so the JIT can optimise aggressively and the absolute figures are inflated. The trustworthy signal is the **shape**: before, throughput *falls* from 26M to 13M as soon as a second thread appears (negative scaling under `synchronized (clazz)`); after, it scales with thread count. The single-threaded 3.6x is the uncontended-lock cost, which is real now that biased locking is gone from modern JVMs.
   
   The benchmark harness was throwaway and is not included in this PR.
   
   ## Behaviour changes
   
   - `XWorkConverter.buildConverterMapping` (`protected`) no longer stores its result; storage is owned by `computeMappingIfAbsent`. `conditionalReload` writes back its own rebuild.
   - `StrutsTypeConverterHolder.addDefaultMapping` now ignores a `null` converter with a warning instead of storing it. `ConcurrentHashMap` forbids null values, and storing one previously left the holder inconsistent — `containsDefaultMapping` true while `getDefaultMapping` returned null. No in-tree caller can pass null today.
   - `addNoMapping` storing the sentinel replaces any mapping previously cached for that class. This matches the pre-7.3.0 *observable* behaviour, where such a class was short-circuited before its cached mapping was ever read. `putIfAbsent` was considered and rejected: it would leave a stale mapping being served after a failed build.
   - `XWorkConverter.registerConverter`/`registerConverterNotFound` are no longer `synchronized`.
   - `TypeConverterHolder.getMapping`, `addMapping` and `containsNoMapping` are deprecated, superseded by `computeMappingIfAbsent`. `addNoMapping` stays — it is still the right primitive for recording a build failure, and folding it in would mean swallowing `Throwable` inside the SPI.
   
   Binary compatibility was deliberately preserved. `StrutsTypeConverterHolder.unknownMappings` keeps its original `protected HashSet<String>` declaration as a deprecated, unused vestige: retyping it changed the field descriptor, so a subclass compiled against 7.2.0 would have hit `NoSuchFieldError` on upgrade without recompiling. Real storage moved to a private concurrent set.
   
   ## Known benign race, deliberately not closed
   
   `lookup()` reads `containsUnknownMapping` then `containsDefaultMapping` as two separate atomic calls. The pair is not atomic, so a converter registered between them yields a stale `null` for that one call, self-correcting on the next lookup. Adding a lock would reintroduce the contention this PR removes, on the hottest read path, to close a window that resolves itself. Note that against the old unsynchronised `HashMap` this same interleaving was outright unsafe.
   
   Relatedly, `lookup()` deliberately keeps check-then-act rather than using `computeIfAbsent`: its resolver `lookupSuper()` reads `getDefaultMapping()` recursively while walking the class hierarchy, and a recursive read inside `ConcurrentHashMap.computeIfAbsent` is forbidden — it deadlocks or throws `IllegalStateException: Recursive update`.
   
   ## Testing
   
   Full core module: 3014 tests, 0 failures, 0 errors. Full multi-module reactor: BUILD SUCCESS. No pre-existing test needed modification.
   
   New concurrency tests use 16 threads released simultaneously via `CountDownLatch`, asserting on collected results rather than on timing. They can demonstrate a race but never prove its absence — a green run on a strongly-ordered machine is weak evidence, and the primary correctness argument is the reasoning about the data structures: reads on concurrent collections, both `computeIfAbsent` call sites verified not to re-enter their own maps, and the remaining races benign and self-correcting.
   
   ## Follow-ups (not in this PR)
   
   - **Classloader pinning.** `mappings` holds strong `Class` references in a container singleton, keeping webapp classloaders alive across hot redeploy. A lifetime/correctness issue rather than a locking one, and fixing it changes cache semantics — deserves its own ticket.
   - **Removing the deprecated methods** in a later release.
   - The `FIXME lukaszlenart` in `TypeConverterHolder` about merging `unknownMappings` into `noMapping` is a semantic consolidation, untouched here.
   




Issue Time Tracking
-------------------

            Worklog Id:     (was: 1031535)
    Remaining Estimate: 0h
            Time Spent: 10m

> Concurrency performance enhancements
> ------------------------------------
>
>                 Key: WW-5539
>                 URL: https://issues.apache.org/jira/browse/WW-5539
>             Project: Struts 2
>          Issue Type: Improvement
>          Components: Core
>    Affects Versions: 6.7.4, 7.0.3
>            Reporter: Kusal Kithul-Godage
>            Assignee: Lukasz Lenart
>            Priority: Minor
>             Fix For: 7.3.0
>
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> We've identified some code which can be made more performant by improving the locking mechanism in the following classes:
>  * XWorkConverter
>  * DefaultActionValidatorManager
>  * StrutsTypeConverterHolder



--
This message was sent by Atlassian Jira
(v8.20.10#820010)