feat: Slight performance improvements#425
Open
kissifrot wants to merge 2 commits intocloudinary:masterfrom
Open
feat: Slight performance improvements#425kissifrot wants to merge 2 commits intocloudinary:masterfrom
kissifrot wants to merge 2 commits intocloudinary:masterfrom
Conversation
7ca7a61 to
184390a
Compare
9 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
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 small performance improvements targeting the most common hot paths: asset construction, URL generation, and API requests. No behaviour change.
BaseAsset::configuration()andAuthToken::configuration(): added a fast path that clones config sections directly when aConfigurationinstance is passed, avoiding a fullnew Configuration(...)rebuild (7-object init + serialize/deserialize cycle) on every asset creation.Configuration::jsonSerialize()andBaseAsset::jsonSerialize(): replacedarray_merge($json, $section)in loops with the union operator+=, equivalent for unique string-keyed arrays.ApiClient::buildHttpClientConfig(): replacedarray_merge_recursivewith an explicit header merge +array_merge, avoiding nested-array side effects and unnecessary overhead.AssetFinalizerTrait::finalizeSource()/finalizeVersion(): replacedpreg_matchcalls withstripos/strncmp(preserving original case-sensitivity per call site); eliminated a redundant double call topublicId()infinalizeVersion().AssetDescriptor::setSuffix(): replacedpreg_match('/[.\/]/')withstr_contains.What does this PR address?
Are tests included?
Reviewer, please note:
BaseAsset::configuration()fast path usescloneinstead of a serialize/deserialize round-trip. Independence from the sourceConfigurationsingleton is covered by the newtestAssetConfigurationIsIndependentFromGlobalConfigtest.finalizeSource()regex/^https?:\//imatches a single slash (e.g.http:/), not://. Thestriposreplacement preserves this exact semantics.finalizeVersion()used a case-sensitive regex;strncmppreserves that, whilefinalizeSource()used/i;stripospreserves that.User-Agentis preserved alongsideAuthorization— this is the regression guard for thebuildHttpClientConfig()change.Edit: I added a benchmark tool, below are the results:
10 000 iterations × 3 runs averaged on each branch (
php tools/benchmark.php).master)new Image($source)(string) new Image($source)$image->toUrl()(pre-built asset)new Image + setSuffix()(string) new Video($source)Configuration::jsonSerialize()new Image($source, $configArray)(array slow path)Main gain comes from the
configuration()fast path: asset construction no longer triggers a fullnew Configuration(...)rebuild (7-object init + serialize/deserialize cycle) on every call.The
toUrl()row on a pre-built asset isolates URL generation improvements independently of construction overhead.Checklist: