perf: avoid array allocations in OTP hot path#283
Open
pataar wants to merge 1 commit into
Open
Conversation
- getParameter() reads $this->parameters directly instead of going through
getParameters(), which builds a new array (and conditionally appends the
issuer) on every call. getParameter is called multiple times per
generateOTP() via getDigest/getSecret/getDigits, so each generation paid
several unnecessary allocations.
- generateOTP() hoists getDigits() to a local so str_pad and the modulo
no longer recompute it.
Behavior is unchanged: callers reading 'issuer' via getParameter() already
threw (hasParameter('issuer') is false because issuer lives in $this->issuer),
and no internal callsite passes 'issuer' or 'label' to getParameter.
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
1 participant
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.
Summary
Two micro-optimizations on the
generateOTP()hot path. No behavior change.getParameter()reads$this->parametersdirectly instead of throughgetParameters(), which copies the array and conditionally appendsissueron every call. Hit 3+ times per generation (foralgorithm,secret,digits).generateOTP()hoistsgetDigits()to a local so the modulo andstr_padwidth no longer recompute it.Combined savings multiply under
TOTP::verify(..., leeway)(3 generations) andHOTP::verify(..., window)(up to N).Why it's safe
getParameters()addsissuerdynamically, butgetParameter('issuer')already threw —hasParameter('issuer')returns false becauseissuerlives in$this->issuer, never in$this->parameters. Same forlabel. Grep confirms no callers pass'issuer'/'label'togetParameter. RFC 4226 / 6238 vector tests cover the full generation pipeline across SHA1/256/512.Verified locally: PHPUnit (160 tests, 440 assertions), PHPStan, ECS, Rector, parallel-lint — all clean.