[
https://issues.apache.org/jira/browse/WW-5667?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107162#comment-18107162
]
Lukasz Lenart edited comment on WW-5667 at 8/23/26 5:36 PM:
------------------------------------------------------------
Resolving. All the work this was decomposed into has landed for 7.4.0.
h3. What shipped
||ticket||change||PR||
|WW-5674|{{isClassBelongsToPackages}} no longer does {{split}} / {{IntStream}}
/ {{String.join}} per call — it walks the package name in place, shortest
prefix first, and {{toPackageName}} uses the cached {{Class.getPackageName()}}
instead of
{{Class.getPackage()}}|[#1830|https://github.com/apache/struts/pull/1830]|
|WW-5675|the parsed security configuration moved to a new {{SINGLETON}} bean,
{{SecurityMemberAccessConfig}}, so it is parsed once at startup instead of on
every {{SecurityMemberAccess}} instantiation. Also fixed
{{ConfigParseUtil.validatePackageNames}} recompiling {{Pattern.compile("\\s")}}
once per package name|[#1847|https://github.com/apache/struts/pull/1847]|
|WW-5676|investigated whether arrays should resolve to their element package;
concluded no change, with tests pinning the
reasoning|[#1858|https://github.com/apache/struts/pull/1858]|
|WW-5677|removed the residual {{getPackage()}} lookups in
{{checkDefaultPackageAccess}} and hoisted the package-name resolution out of
the {{isExcludedPackageNamePatterns}}
loop|[#1859|https://github.com/apache/struts/pull/1859]|
WW-5678 remains Open against 8.0.0. It is a naming and visibility tidy-up with
no runtime effect, deliberately kept out of this bug: part of it is
source-breaking, so it cannot ship in a minor release.
h3. On the proposed fix
Jose Miguel, thank you for the report — the two JFR stack samples were what
made this tractable, and they turned out to be two independent problems rather
than one.
The fix suggested in the report, caching the parsed {{Set}} in a
{{SecurityMemberAccess}} field, would not have resolved either of them, and it
is worth recording why:
* it does not touch the first sample's hot path, which was the per-call
allocation inside {{isClassBelongsToPackages}} (WW-5674); and
* it could not help the second, because {{SecurityMemberAccess}} is
{{Scope.PROTOTYPE}} — the instance holding the cached field is discarded and
rebuilt on every container lookup, so the cache would never be hit. Moving the
parsed state to a singleton bean that the prototype reads from (WW-5675) is
what actually removes that cost.
h3. One request
We have not re-run your profile. The changes are structural — the parsing and
the allocations are provably gone from those paths, and the full core suite is
green — but the 9% figure came from your workload, not ours, so we cannot
honestly claim a measured improvement against it.
If you are able to re-measure once 7.4.0 is released, we would be glad to hear
the result. If the cost has not moved as expected, please reopen or file a
follow-up with the new profile.
was (Author: lukaszlenart):
Resolving. All the work this was decomposed into has landed for 7.4.0.
h3. What shipped
||ticket||change||PR||
|WW-5674|{{isClassBelongsToPackages}} no longer does {{split}} / {{IntStream}}
/ {{String.join}} per call — it walks the package name in place, shortest
prefix first, and {{toPackageName}} uses the cached {{Class.getPackageName()}}
instead of
{{Class.getPackage()}}|[#1830|https://github.com/apache/struts/pull/1830]|
|WW-5675|the parsed security configuration moved to a new {{SINGLETON}} bean,
{{SecurityMemberAccessConfig}}, so it is parsed once at startup instead of on
every {{SecurityMemberAccess}} instantiation. Also fixed
{{ConfigParseUtil.validatePackageNames}} recompiling {{Pattern.compile("\\s")}}
once per package name|[#1847|https://github.com/apache/struts/pull/1847]|
|WW-5676|investigated whether arrays should resolve to their element package;
concluded no change, with tests pinning the
reasoning|[#1858|https://github.com/apache/struts/pull/1858]|
|WW-5677|removed the residual {{getPackage()}} lookups in
{{checkDefaultPackageAccess}} and hoisted the package-name resolution out of
the {{isExcludedPackageNamePatterns}}
loop|[#1859|https://github.com/apache/struts/pull/1859]|
WW-5678 remains Open against 8.0.0. It is a naming and visibility tidy-up with
no runtime effect, deliberately kept out of this bug: part of it is
source-breaking, so it cannot ship in a minor release.
h3. On the proposed fix
[~josemiguel1999], thank you for the report — the two JFR stack samples were
what made this tractable, and they turned out to be two independent problems
rather than one.
The fix suggested in the report, caching the parsed {{Set}} in a
{{SecurityMemberAccess}} field, would not have resolved either of them, and it
is worth recording why:
* it does not touch the first sample's hot path, which was the per-call
allocation inside {{isClassBelongsToPackages}} (WW-5674); and
* it could not help the second, because {{SecurityMemberAccess}} is
{{Scope.PROTOTYPE}} — the instance holding the cached field is discarded and
rebuilt on every container lookup, so the cache would never be hit. Moving the
parsed state to a singleton bean that the prototype reads from (WW-5675) is
what actually removes that cost.
h3. One request
We have not re-run your profile. The changes are structural — the parsing and
the allocations are provably gone from those paths, and the full core suite is
green — but the 9% figure came from your workload, not ours, so we cannot
honestly claim a measured improvement against it.
If you are able to re-measure once 7.4.0 is released, we would be glad to hear
the result. If the cost has not moved as expected, please reopen or file a
follow-up with the new profile.
> Performance Issue: SecurityMemberAccess re-parses excluded packages/classes
> config string on every OGNL access — O(n) String.split per request
> ----------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: WW-5667
> URL: https://issues.apache.org/jira/browse/WW-5667
> Project: Struts 2
> Issue Type: Bug
> Reporter: Jose Miguel
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
>
> In stress test is consuming 9% of a server with 4 CPUs.
> **Struts version:** 7.2.1
> **JDK:** 21 (OpenJDK ms-21.0.7)
> **JVM flags:** Payara 7.2026.6
> **Summary:**
> `SecurityMemberAccess.isClassBelongsToPackages()` and the related
> `useExcludedPackageNames()` / `useExcludedClasses()` methods call
> `ConfigParseUtil.toNewPackageNamesSet()` / `toNewClassesSet()` on every
> OGNL member-access check, which calls `String.split()` and builds a new
> `HashSet` each time from the raw config string.
> Under moderate load (preprod server, ~20% Payara CPU) this accounts for
> ~9% of all RUNNABLE CPU samples in a 2-minute JFR profile.
> **Stack traces (JFR ExecutionSamples, STATE_RUNNABLE):**
> Sample 1:
> java.lang.String.split(String)
> SecurityMemberAccess.isClassBelongsToPackages(Class, Set) :390
> SecurityMemberAccess.isExcludedPackageNames(Class) :386
> SecurityMemberAccess.isPackageExcluded(Class) :371
> Sample 2:
> HashMap.put / HashSet.add
> ConfigParseUtil.toNewPackageNamesSet(Collection, String) :138
> SecurityMemberAccess.useExcludedPackageNames(String) :446
> **Proposed fix:**
> Cache the parsed `Set<String>` in `SecurityMemberAccess` as a field.
> Rebuild it only when the raw config string reference changes (compare
> with `==` or store the last-seen string). Since the excluded-packages
> config is set once at startup and never changes at runtime, the cache
> never needs invalidation in practice.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
