feat: Slight performance improvements#29
Open
kissifrot wants to merge 1 commit intocloudinary:mainfrom
Open
Conversation
Author
|
@const-cloudinary if you happen to pass by 😇 🙏 |
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
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.
Brief Summary of Changes
Several targeted performance improvements on the serialization hot path (qualifier key/name resolution, multi-value qualifier rendering), plus a regression test and expanded coverage for
isJsonString().No external behavior change.
What does this PR address?
Are tests included?
Details
QualifierMultiValue::__toString()— mutation bug fix (biggest gain)EffectNameargument keys were appended directly to$this->argumentOrderon every call. Any code path serializing the same qualifier more than once (URL generation loops, logging, test assertions) caused$argumentOrderto grow unboundedly, degrading sort performance on subsequent calls.Fix: operate on a local copy of
$argumentOrderinside__toString().BaseQualifier::getKey()— per-class static cachegetKey()invokedgetName()+StringUtils::toAcronym()on every call.The result depends solely on the class name and is immutable per process.
Added a
private static array $keyCachekeyed bystatic::class(Late Static Binding) so each concrete subclass pays the derivation cost once.BaseComponent::getName()— per-class static cacheSame rationale:
getName()calledClassUtils::getBaseName()+StringUtils::camelCaseToSnakeCase()on every invocation. Addedprivate static array $nameCachewith the same LSB pattern.JsonUtils::isJsonString()— fast path + PHP 8.3+{or[) before any parsing, preserving the original semantics of accepting only JSON objects and arrays.json_validate(), which validates JSON syntax without building a decoded value in memory.Benchmark
isJsonString()— large payloadReviewer, please note:
$nameCache/$keyCachearrays live on the respective base classes and are keyed bystatic::class, so each concrete subclass gets its own entry while the storage stays on the base class. The caches are per-process (transparently reset on each PHP-FPM request).QualifierMultiValuechange is a pure bug fix — the old code produced incorrect sort order after the first__toString()call on any instance whoseEffectNamearguments were not pre-sorted. A regression test is included inEffectTest::testEffectQualifierToStringIsIdempotent().isJsonString()behaviour is unchanged with one narrow exception: strings with leading whitespace before{/[(e.g." {}") now returnfalse.This was not a supported use-case in practice (configs are always programmatically generated without leading whitespace), but indicate me if I need to alter it 👍
Checklist: