perf: close BE request-lifecycle regression vs 8.1 stable - #701
Merged
Merged
Conversation
Root-caused and fixed five per-request hot-path overheads introduced since 8.1 that were adding up across every request: - HandlerService: getRouteCachingMetadata() was resolved twice per request (once from getEventMetadataEntry(), once from getEventCachingMetadata()) for a route record that cannot change for the life of a request. Memoized in `request` scope so it's resolved once. - HandlerService: the cache-key-suffix dynamic-vs-static check (isClosure()/isCustomFunction()/isSimpleValue()) was recomputed via function calls on every cache-metadata lookup. Precomputed once as a boolean field when the dictionary entry is built instead. - BaseService.getLogger(): dropped a redundant structKeyExists() check ahead of isNull() - the declared `log` property already guarantees the variables-scope slot exists. - InterceptorState (a separate class hierarchy from BaseService, so it independently reimplemented the same lazy-getter pattern): resolve the logger eagerly in init() instead of lazily on every call. - InterceptorService.getLazyBuffer(): was allocating a new InterceptorBuffer component on every announce() call (10+ per request). Pooled per request with checkout/release, falling back to an unpooled instance for the async/asyncAll paths (whose buffer can outlive the announce() call on a background thread) and for a reentrant announce() call (e.g. onException triggered from within an interceptor), so a nested call can never corrupt a buffer still in flight further up the call stack. Validated functionally against live BoxLang and Lucee 7 servers (all 5 perf-harness scenarios plus an invalid-event/exception path to exercise interceptor reentrancy) and via repeated paired BE/8.1-stable throughput runs on boxlang, boxlang-cfml, and lucee-7. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
…tructs
getEventMetadataEntry()'s fast path and resolveCacheSuffix() assumed
every mdEntry-shaped struct carries the precomputed suffixIsDynamic
field added in the previous commit. That's only true for entries built
by getEventCachingMetadata()'s dictionary-population path - a struct
built any other way (e.g. EventCachingSpec's direct call to
resolveCacheSuffix() with a bare {suffix, cacheable} struct) doesn't
have it, and BoxLang throws KeyNotFoundException on the direct-access
read instead of just treating it as undefined.
Fall back to computing it inline via the same Elvis-with-missing-key
idiom already used elsewhere in this codebase (LuceeMappingHelper.cfc,
RestHandler.cfc) - verified directly against live BoxLang and Lucee 7
instances that struct.missingKey ?: default does not throw and
evaluates the fallback.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
…uest scope getRouteCachingMetadata() memoized its result on the raw CFML `request` scope, assuming it's one-per-logical-request. That's true for a real HTTP request but false in TestBox: the whole spec suite runs inside a single physical HTTP request, so the first HandlerServiceTest case to call getRouteCachingMetadata() poisoned every subsequent case in that describe block with its own route record's result, regardless of the route record each later test actually passed in. Moved the memo key onto the requestContext instance instead, via the same setPrivateValue()/getPrivateValue()/privateValueExists() methods RequestContext already uses to store currentRouteRecord. That struct (variables.privateContext) is a genuine instance property re-created fresh in every init(), so it's isolated per object by the CFML/BoxLang object model itself - not dependent on any engine-specific behavior. Tests build a fresh mock RequestContext per case (BaseTestCase.setup() calls removeContext() in a beforeEach), so this scopes the memo exactly where it needs to be: once per real (or test) request, never leaking across cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
Member
Author
|
One This isn't this PR's failure — the stack trace only touches Generated by Claude Code |
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to the perf-harness work in #700, which surfaced a consistent throughput regression on ColdBox BE (8.2-dev) vs 8.1 stable across engines. This PR fixes five per-request hot-path overheads found by diffing v8.1.0 against
developmentand validating each candidate with paired BE/stable throughput measurements before turning it into a real fix.Fixes
HandlerService.getRouteCachingMetadata()(route-level.withCache()support) was resolved twice per request — once fromgetEventMetadataEntry()(pre-dispatch cacheability check) and again fromgetEventCachingMetadata()(at dispatch) — for a route record that cannot change for the life of a request. Memoized inrequestscope so it's resolved once.isClosure()/isCustomFunction()/isSimpleValue()) was recomputed via function calls on every cache-metadata lookup. Precomputed once as a boolean field (suffixIsDynamic) when the dictionary entry is built instead.BaseService.getLogger()dropped a redundantstructKeyExists()check ahead ofisNull()— the declaredlogproperty already guarantees the variables-scope slot exists.InterceptorState(extendsEventPool, a separate class hierarchy fromBaseService, so it had independently reimplemented the same lazy-getter pattern) now resolves the logger eagerly ininit()instead of lazily on every call.InterceptorService.getLazyBuffer()was allocating a newInterceptorBuffercomponent on everyannounce()call (10+ per request). Pooled per request with checkout/release, falling back to an unpooled instance for theasync/asyncAllpaths (whose buffer can outlive theannounce()call on a background thread) and for a reentrantannounce()call (e.g.onExceptiontriggered from within an interceptor), so a nested call can never corrupt a buffer still in flight further up the call stack.Validation
onInvalidEvent/onException, exercisingannounce()reentrancy) against live BoxLang and Lucee 7 servers — all correct, no errors.Type of change
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
Generated by Claude Code