[
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 excluded class names from {{struts.excludedClasses}} and another 16 from
{{struts.devMode.excludedClasses}} — comma split plus classloader validation
* 29 excluded package names from {{struts.excludedPackageNames}} and another 29
from {{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. Design note: this ticket unblocks collapsing the allowlist two-set walk
WW-5674 merged the two allowlist walks in {{isClassAllowlisted}} by adding a
two-set helper:
{code:java}
static boolean isPackageBelongsToPackages(String packageName, Set first, Set
second)
{code}
The cleaner shape is a single set holding the union of
{{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}}, precomputed
once. That would delete the two-set parameter and the three-argument overload
entirely, leaving one set and one walk.
It was rejected in WW-5674 for two reasons, and _this ticket removes the first
and largely removes the second_:
h1. Under the current prototype scope, a precomputed union field is rebuilt on
every instantiation — several times per request — adding to exactly the cost
this ticket is about. Once the parsed configuration is shared rather than
per-instance, the union becomes free.
h1. The union would need initialising in two places: at the field declaration,
for the case where the {{@Inject}} setter never fires because no allowlist is
configured, and again in the setter. Duplicated initialisation here is a
_fail-open_ hazard — get it wrong and {{ALLOWLIST_REQUIRED_PACKAGES}} silently
drops out of the allowlist with nothing failing loudly. Whatever this ticket
does to give the configuration a single well-defined construction point makes
that much safer to get right.
Worth doing as part of this ticket rather than leaving it to WW-5678, since the
enabling change lands here. Note that it would also resolve WW-5678's first
item for free, by removing the package-private overload that shares a name with
a public method.
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). Handled separately; it does not
move this cost.
* WW-5677 — remaining redundant {{getPackage()}} lookups on the per-access
path. Same file, different root cause.
* WW-5678 — helper naming and visibility cleanup for 8.0.0. Its first item may
be resolved for free here, see the design note above.
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}}
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.
> 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 excluded class names from {{struts.excludedClasses}} and another 16 from
> {{struts.devMode.excludedClasses}} — comma split plus classloader validation
> * 29 excluded package names from {{struts.excludedPackageNames}} and another
> 29 from {{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. Design note: this ticket unblocks collapsing the allowlist two-set walk
> WW-5674 merged the two allowlist walks in {{isClassAllowlisted}} by adding a
> two-set helper:
> {code:java}
> static boolean isPackageBelongsToPackages(String packageName, Set first, Set
> second)
> {code}
> The cleaner shape is a single set holding the union of
> {{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}}, precomputed
> once. That would delete the two-set parameter and the three-argument overload
> entirely, leaving one set and one walk.
> It was rejected in WW-5674 for two reasons, and _this ticket removes the
> first and largely removes the second_:
> h1. Under the current prototype scope, a precomputed union field is rebuilt
> on every instantiation — several times per request — adding to exactly the
> cost this ticket is about. Once the parsed configuration is shared rather
> than per-instance, the union becomes free.
> h1. The union would need initialising in two places: at the field
> declaration, for the case where the {{@Inject}} setter never fires because no
> allowlist is configured, and again in the setter. Duplicated initialisation
> here is a _fail-open_ hazard — get it wrong and
> {{ALLOWLIST_REQUIRED_PACKAGES}} silently drops out of the allowlist with
> nothing failing loudly. Whatever this ticket does to give the configuration a
> single well-defined construction point makes that much safer to get right.
> Worth doing as part of this ticket rather than leaving it to WW-5678, since
> the enabling change lands here. Note that it would also resolve WW-5678's
> first item for free, by removing the package-private overload that shares a
> name with a public method.
> 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). Handled separately; it does not
> move this cost.
> * WW-5677 — remaining redundant {{getPackage()}} lookups on the per-access
> path. Same file, different root cause.
> * WW-5678 — helper naming and visibility cleanup for 8.0.0. Its first item
> may be resolved for free here, see the design note above.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
