[
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)
