[jira] [Updated] (WW-5675) Stop re-parsing OGNL security config on every SecurityMemberAccess instantiation

["Lukasz Lenart (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/WW-5675?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Lukasz Lenart updated WW-5675:
------------------------------
    Description: 
Sub-task of WW-5667.

{{SecurityMemberAccess}} is a {{Scope.PROTOTYPE}} bean ({{StrutsBeanSelectionProvider}}, since WW-5343). Every {{container.getInstance(SecurityMemberAccess.class)}} therefore constructs a fresh instance and re-runs all of its {{@Inject}} setters, each of which re-parses a raw comma-delimited configuration string from scratch.

With the stock {{struts-excluded-classes.xml}}, a single instantiation re-parses roughly:
* 16 + 16 excluded class names ({{struts.excludedClasses}}, {{struts.devMode.excludedClasses}}) — comma split plus classloader validation
* 29 + 29 excluded package names ({{struts.excludedPackageNames}}, {{struts.devMode.excludedPackageNames}}) — comma split, {{strip}}, and {{validatePackageNames}}
* the allowlist class and package sets
* any configured excluded-package-name patterns, which are re-compiled via {{Pattern.compile}}

New instances are created on the request path from at least:
* {{OgnlValueStackFactory.createValueStack(...)}} — once per value stack; {{ParametersInterceptor.toNewStack}} creates an additional stack per request
* {{OgnlUtil.createDefaultContext(Object, ClassResolver)}} — reached from {{setProperties}}, {{copy}}, {{getBeanMap}} and friends

so the full configuration is rebuilt several times per request.

This matches JFR sample 2 on the parent ticket:

{noformat}
HashMap.put / HashSet.add
ConfigParseUtil.toNewPackageNamesSet(Collection, String) :138
SecurityMemberAccess.useExcludedPackageNames(String) :446
{noformat}

Note that the fix proposed on the parent ticket — caching the parsed set in a {{SecurityMemberAccess}} field — does not address this, because the instance holding the field is itself discarded and rebuilt each time.

h2. Also in scope: {{validatePackageNames}} recompiles a constant pattern per entry

{{ConfigParseUtil.validatePackageNames}} evaluates {{Pattern.compile("\\s")}} once per package name rather than once overall:

{code:java}
public static void validatePackageNames(Collection packageNames) {
    if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) {
        throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames);
    }
}
{code}

With the default configuration that is roughly 58 recompiles of a trivial pattern per {{SecurityMemberAccess}} instantiation, on top of the parsing work above. Hoisting the pattern to a static constant — or replacing the check with a plain character scan — is a small, self-contained fix, and it shares this ticket's root cause of doing per-instantiation work that should be done once.

h2. Constraints

The OGNL allow/deny semantics must not change. The dev-mode configuration switchover in {{useDevModeConfiguration()}} must keep working. Per-instance mutable state set at request time by {{ParametersInterceptor.applyMemberAccessProperties}} ({{useAcceptProperties}} / {{useExcludeProperties}}) must stay per-instance — this is why the bean was made prototype-scoped in the first place, so any move back towards sharing has to account for it.

h2. Related
* WW-5674 — the per-access half of the parent report ({{isClassBelongsToPackages}} allocations). Merged separately; it does not move this cost.
* WW-5677 — remaining redundant {{getPackage()}} lookups on the per-access path. Same file, different root cause.

  was:
Sub-task of WW-5667.

{{SecurityMemberAccess}} is a {{Scope.PROTOTYPE}} bean ({{StrutsBeanSelectionProvider}}, since WW-5343). Every {{container.getInstance(SecurityMemberAccess.class)}} therefore constructs a fresh instance and re-runs all of its {{@Inject}} setters, each of which re-parses a raw comma-delimited configuration string from scratch.

With the stock {{struts-excluded-classes.xml}}, a single instantiation re-parses roughly:
* 16 + 16 excluded class names ({{struts.excludedClasses}}, {{struts.devMode.excludedClasses}}) — comma split plus classloader validation
* 29 + 29 excluded package names ({{struts.excludedPackageNames}}, {{struts.devMode.excludedPackageNames}}) — comma split, {{strip}}, and {{validatePackageNames}}
* the allowlist class and package sets
* any configured excluded-package-name patterns, which are re-compiled via {{Pattern.compile}}

{{ConfigParseUtil.validatePackageNames}} additionally evaluates {{Pattern.compile("\\s")}} once per package name rather than once overall, so a default configuration recompiles that trivial pattern around 58 times per instance.

New instances are created on the request path from at least:
* {{OgnlValueStackFactory.createValueStack(...)}} — once per value stack; {{ParametersInterceptor.toNewStack}} creates an additional stack per request
* {{OgnlUtil.createDefaultContext(Object, ClassResolver)}} — reached from {{setProperties}}, {{copy}}, {{getBeanMap}} and friends

so the full configuration is rebuilt several times per request.

This matches JFR sample 2 on the parent ticket:

{noformat}
HashMap.put / HashSet.add
ConfigParseUtil.toNewPackageNamesSet(Collection, String) :138
SecurityMemberAccess.useExcludedPackageNames(String) :446
{noformat}

Note that the fix proposed on the parent ticket — caching the parsed set in a {{SecurityMemberAccess}} field — does not address this, because the instance holding the field is itself discarded and rebuilt each time.

Constraints for any fix: the OGNL allow/deny semantics must not change; the dev-mode configuration switchover in {{useDevModeConfiguration()}} must keep working; and per-instance mutable state set at request time by {{ParametersInterceptor.applyMemberAccessProperties}} ({{useAcceptProperties}} / {{useExcludeProperties}}) must stay per-instance.


> Stop re-parsing OGNL security config on every SecurityMemberAccess instantiation
> --------------------------------------------------------------------------------
>
>                 Key: WW-5675
>                 URL: https://issues.apache.org/jira/browse/WW-5675
>             Project: Struts 2
>          Issue Type: Sub-task
>            Reporter: Lukasz Lenart
>            Priority: Major
>
> Sub-task of WW-5667.
> {{SecurityMemberAccess}} is a {{Scope.PROTOTYPE}} bean ({{StrutsBeanSelectionProvider}}, since WW-5343). Every {{container.getInstance(SecurityMemberAccess.class)}} therefore constructs a fresh instance and re-runs all of its {{@Inject}} setters, each of which re-parses a raw comma-delimited configuration string from scratch.
> With the stock {{struts-excluded-classes.xml}}, a single instantiation re-parses roughly:
> * 16 + 16 excluded class names ({{struts.excludedClasses}}, {{struts.devMode.excludedClasses}}) — comma split plus classloader validation
> * 29 + 29 excluded package names ({{struts.excludedPackageNames}}, {{struts.devMode.excludedPackageNames}}) — comma split, {{strip}}, and {{validatePackageNames}}
> * the allowlist class and package sets
> * any configured excluded-package-name patterns, which are re-compiled via {{Pattern.compile}}
> New instances are created on the request path from at least:
> * {{OgnlValueStackFactory.createValueStack(...)}} — once per value stack; {{ParametersInterceptor.toNewStack}} creates an additional stack per request
> * {{OgnlUtil.createDefaultContext(Object, ClassResolver)}} — reached from {{setProperties}}, {{copy}}, {{getBeanMap}} and friends
> so the full configuration is rebuilt several times per request.
> This matches JFR sample 2 on the parent ticket:
> {noformat}
> HashMap.put / HashSet.add
> ConfigParseUtil.toNewPackageNamesSet(Collection, String) :138
> SecurityMemberAccess.useExcludedPackageNames(String) :446
> {noformat}
> Note that the fix proposed on the parent ticket — caching the parsed set in a {{SecurityMemberAccess}} field — does not address this, because the instance holding the field is itself discarded and rebuilt each time.
> h2. Also in scope: {{validatePackageNames}} recompiles a constant pattern per entry
> {{ConfigParseUtil.validatePackageNames}} evaluates {{Pattern.compile("\\s")}} once per package name rather than once overall:
> {code:java}
> public static void validatePackageNames(Collection packageNames) {
>     if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) {
>         throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames);
>     }
> }
> {code}
> With the default configuration that is roughly 58 recompiles of a trivial pattern per {{SecurityMemberAccess}} instantiation, on top of the parsing work above. Hoisting the pattern to a static constant — or replacing the check with a plain character scan — is a small, self-contained fix, and it shares this ticket's root cause of doing per-instantiation work that should be done once.
> h2. Constraints
> The OGNL allow/deny semantics must not change. The dev-mode configuration switchover in {{useDevModeConfiguration()}} must keep working. Per-instance mutable state set at request time by {{ParametersInterceptor.applyMemberAccessProperties}} ({{useAcceptProperties}} / {{useExcludeProperties}}) must stay per-instance — this is why the bean was made prototype-scoped in the first place, so any move back towards sharing has to account for it.
> h2. Related
> * WW-5674 — the per-access half of the parent report ({{isClassBelongsToPackages}} allocations). Merged separately; it does not move this cost.
> * WW-5677 — remaining redundant {{getPackage()}} lookups on the per-access path. Same file, different root cause.



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