Skip to content

feat: increase RandomCache size from 256 to 4096 bytes#51

Merged
larseggert merged 1 commit into
mainfrom
perf/larger-random-cache
May 11, 2026
Merged

feat: increase RandomCache size from 256 to 4096 bytes#51
larseggert merged 1 commit into
mainfrom
perf/larger-random-cache

Conversation

@larseggert
Copy link
Copy Markdown
Collaborator

random::<N>() refills a thread-local cache via PK11_GenerateRandom (SHA-256 DRBG) each time the cache is exhausted. That can still cause measurable overheads if a lot of random bytes are needed.

`random::<N>()` refills a thread-local cache via `PK11_GenerateRandom`
(SHA-256 DRBG) each time the cache is exhausted. That can still
cause measurable overheads if a lot of random bytes are needed.
Copilot AI review requested due to automatic review settings May 8, 2026 11:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR increases the thread-local RandomCache buffer used by random::<N>() (for N <= 32) to reduce the frequency of calls into NSS (PK11_GenerateRandom) when many small random values are requested.

Changes:

  • Increase RandomCache::SIZE from 256 bytes to 4096 bytes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Clean, minimal change — the constant propagates correctly through RandomCache::new(), randomize(), and the thread-local, so correctness is not a concern.

Nit on PR title: the commit message uses perf: (matching the branch name perf/larger-random-cache), but the PR title says feat:. Consider updating the title to perf: for consistency.

Benchmark data: a 16× buffer increase (256 → 4096) is a reasonable trade-off of ~3.75 KB extra thread-local memory per thread for fewer PK11_GenerateRandom calls, but the PR description says the overhead is "measurable" without providing measurements. Adding before/after numbers (even a quick micro-benchmark) would help reviewers judge whether 4096 is the right landing point vs. 1024 or 2048, and give future readers context for why this value was chosen.

@martinthomson
Copy link
Copy Markdown
Member

I'd like to see some evidence of a performance improvement out of this before making the cut.

I'd also like to see copilot review a PR without saying "clean", just once.

larseggert added a commit to larseggert/neqo that referenced this pull request May 11, 2026
larseggert added a commit to larseggert/neqo that referenced this pull request May 11, 2026
@larseggert
Copy link
Copy Markdown
Collaborator Author

@larseggert larseggert merged commit 598c094 into main May 11, 2026
49 checks passed
@larseggert larseggert deleted the perf/larger-random-cache branch May 11, 2026 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants