FreeAndNil opened a new pull request, #304:
URL: https://github.com/apache/logging-log4net/pull/304
The cache added in 2.0.15 (commit 9305ea9b) was declared as an instance field
on `LoggingEvent`. Since every event is a fresh instance the guard was always
false, so the `??=` never hit and each event ran a full
`WindowsIdentity.GetCurrent().Name` - measured at 79.6 us on the machine used
here. The same commit had changed `TryGetCurrentUserName` from static to
instance, so the caching it introduced has never taken effect.
The doc comment above the property already described the intended design and
the reason it was abandoned: the name should be cached "as long as the
identity
stayed constant", but `WindowsIdentity.GetCurrent()` "seems to return
different
objects every time". Object identity was the wrong comparison. The security
identifier is stable, and obtaining the identity is far cheaper than
resolving
its name - the timing table in that same comment puts the two at roughly 20
ns
against 804 ns.
So the name is now cached without changing what the property reports:
- A thread that is not impersonating runs as the process identity, so its
name
is resolved once per process. `WindowsIdentity.GetCurrent(ifImpersonating:
true)` answers that question for 249 ns against 78 us for a name, making
this
the fast path for services, console applications and ASP.NET Core.
- A thread that is impersonating - classic ASP.NET with
`<identity impersonate="true"/>`, or `RunImpersonated` - has its name
resolved once per distinct user, keyed by security identifier and capped at
`MaxCachedUserNames` entries so that a site in front of a large directory
cannot accumulate one entry per visitor.
The process-identity value is assigned in exactly one place, inside the
branch
that has already established the thread is not impersonating. Seeding it from
an impersonating thread would report that user for the rest of the process,
which `ImpersonationDoesNotSeedTheProcessUserName` guards against.
A buffered `FixFlags.All` event goes from 192,937 to 17,578 ns on this
machine,
both sides built as netstandard2.0. The remainder is `FixFlags.LocationInfo`,
which is untouched here.
Two related changes in the same method:
- `Environment.UserName`, the fallback when `WindowsIdentity` is unusable, is
also cached; it measured 33.8 us per call. Impersonation does not apply on
that path.
- The `SecurityException` handler now sets the unavailable flag, as the
`PlatformNotSupportedException` handler already did. Under partial trust
the
old code threw, caught and logged once per event forever.
Documentation: the `%username` pattern entry now explains the caching and
points ASP.NET users at `%identity`, which is both cheaper and usually what
is
wanted because it reports the authenticated application user. The
`BufferingForwardingAppender` manual page recommends `Partial` in its example
instead of `All`; the surrounding comment already warned that the `All`
default
"may negatively impact performance enough to warrant changing it", and
`LocationInfo` costs 6.9 us and 14 kB per event.
`LogicalThreadContextProperties` no longer stores an empty dictionary just to
replace it on the next line, and no longer clones on removal of an absent
key.
Both are cleanups; neither changes the allocation profile of the common
set/remove pattern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
