Skip to content

perf: general optimizations - #295

Merged
xaevik merged 6 commits into
mainfrom
performance-optimizations
Sep 20, 2026
Merged

xaevik merged 6 commits into
mainfrom
performance-optimizations

Conversation

@xaevik

@xaevik xaevik commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI lite review requested due to automatic review settings September 20, 2026 11:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Moderate findings remain for the untested no-GMP fallback, radix coupling, and contradictory AGENTS.md guidance.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 2 Low severity

Open (3)
What changed in this PR

Optimizes CUID2 generation by converting raw SHA3-512 digests directly to base36, with expanded tests, benchmarks, and documentation.

Changes:

  • Adds limb-based binary-to-base36 conversion with GMP and pure-PHP paths.
  • Routes raw digest conversion through optimized logic.
  • Expands tests and benchmarks; updates documentation and guidance.
File Summary
tests/​UtilsTest.php Adds byte-conversion coverage.
tests/​FingerprintTest.php Reorganizes and extends fingerprint tests.
tests/​Cuid2Test.php Expands generation and validation tests.
tests/​CounterTest.php Reorganizes counter tests.
tests/​benchmark/​ValidationBench.php Reorganizes validation benchmarks.
tests/​benchmark/​UtilsBench.php Adds conversion benchmarks. Nit (1 vote): use a neutral label instead of fast-path-7-bytes.
tests/​benchmark/​SingletonBench.php Reorganizes singleton benchmarks.
tests/​benchmark/​Cuid2Bench.php Adds generation and serialization benchmarks. Nit (2 votes): move $cuid below setUp().
src/​Utils.php Adds optimized byte/limb conversion. Moderate (1 vote): derive the radix from BASE36_DIGITS_PER_PASS or enforce an invariant.
src/​Fingerprint.php Reorders singleton methods.
src/​Cuid2.php Uses GMP or pure-PHP raw-digest conversion. Moderate (3 votes): add coverage for the no-GMP fallback path.
src/​Counter.php Reorders singleton methods.
README.md Documents conversion and performance. Nits: align benchmark inputs with the table (2 votes) and correct the inconsistent documented range (1 vote).
ARCHITECTURE.md Documents the updated conversion paths.
AGENTS.md Updates repository guidance. Moderate (1 vote): clarify whether the cache restriction applies to internal caches.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Cuid2.php
Comment thread README.md Outdated
Comment thread tests/benchmark/Cuid2Bench.php Outdated
Copilot AI review requested due to automatic review settings September 20, 2026 12:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Three moderate findings and one benchmark-label nit remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
Resolved since last review (3)

Comment thread tests/Cuid2Test.php
Comment thread tests/Support/ExtensionAvailability.php Outdated
Signed-off-by: Alan Brault <[email protected]>
Copilot AI review requested due to automatic review settings September 20, 2026 12:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Three unresolved moderate findings concern benchmark overhead, missing fixed-digest coverage, and unsafe getenv() handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add fixed-digest assertion for GMP conversion correctness

src/​Cuid2.php:246

The new GMP conversion branch is exercised by ordinary generation tests, but no test asserts its result for a fixed digest. An endian or word-order error here would still produce valid-looking, correctly sized CUIDs and pass the current assertions; add a fixed-digest equality check against the independently tested Utils::bytesToBase36() path.

Comment thread tests/Support/ExtensionAvailability.php Outdated
Signed-off-by: Alan Brault <[email protected]>
Copilot AI review requested due to automatic review settings September 20, 2026 12:25
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Moderate GMP-path coverage and the listed style and test-order fixes remain unresolved.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Compare GMP output with forced fallback for fixed digest

tests/​Cuid2Test.php:108

This test disables GMP before invoking convert(), while the other CUID tests assert only format or length. No test now verifies that the new GMP import produces the same value as the pure-PHP path for one digest, so a byte-order or flag regression could pass; capture the GMP result first and compare it with the forced fallback.

@xaevik
xaevik merged commit 709ee3b into main Sep 20, 2026
13 checks passed
@xaevik
xaevik deleted the performance-optimizations branch September 20, 2026 12:32
@visus-io visus-io locked as resolved and limited conversation to collaborators Sep 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants