Skip to content

Conversation

cuiweixie
Copy link
Contributor

@cuiweixie cuiweixie commented Sep 22, 2025

Summary by CodeRabbit

  • Performance
    • Optimized internal byte conversion in pruning operations to reduce memory allocations and improve throughput, especially on large datasets.
  • Refactor
    • Streamlined implementation to preallocate exact buffer sizes, improving efficiency without altering behavior or outputs.

@cuiweixie
Copy link
Contributor Author

cuiweixie commented Sep 22, 2025

#25356
@aljo242

Copy link
Contributor

coderabbitai bot commented Sep 22, 2025

📝 Walkthrough

Walkthrough

Refactors int64SliceToBytes in store/pruning/manager.go to preallocate the exact output byte slice and write each int64’s big-endian bytes directly at computed offsets, replacing per-element append-based construction. No public APIs changed; behavior remains the same.

Changes

Cohort / File(s) Summary
Pruning serialization optimization
store/pruning/manager.go
Rewrote int64 slice-to-bytes conversion to preallocate len*8 buffer and fill via offset indexing (i<<3) with big-endian writes, removing per-iteration appends and reducing allocations; no semantic or control-flow changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "refactor: less alloc" clearly indicates the PR's main intent—reducing allocations—which matches the refactor of int64SliceToBytes described in the changeset; it is concise and related to the primary change, though slightly informal and not function-specific.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
store/pruning/manager.go (2)

227-231: Allocation-free encoding is correct; minor readability tweak suggested

Change reduces allocations and keeps endianness consistent. Consider making the 8‑byte window explicit and using multiplication for clarity.

 func int64SliceToBytes(slice ...int64) []byte {
-	bz := make([]byte, len(slice)*8)
-	for i, ph := range slice {
-		binary.BigEndian.PutUint64(bz[i<<3:], uint64(ph))
-	}
+	bz := make([]byte, len(slice)*8)
+	for i, ph := range slice {
+		off := i * 8 // clearer than i<<3
+		binary.BigEndian.PutUint64(bz[off:off+8], uint64(ph))
+	}
 	return bz
 }

226-231: Add int64ToBytes(int64) and use it from storePruningSnapshotHeight

Implement a helper that encodes a single int64 (no variadic allocation) and call it from store/pruning/manager.go (storePruningSnapshotHeight — currently calls int64SliceToBytes at line 199); keep int64SliceToBytes(...) for multi-value callers (store/pruning/manager_test.go at line 439).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9171179 and 7723a56.

📒 Files selected for processing (1)
  • store/pruning/manager.go (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary

@aljo242
Copy link
Contributor

aljo242 commented Sep 23, 2025

is there a test that validates this change is ok?

@cuiweixie
Copy link
Contributor Author

here will test.

err = db.Set(pruneSnapshotHeightsKey, int64SliceToBytes(tc.getFlushedPruningSnapshotHeights()...))

package main

import (
	"bytes"
	"encoding/binary"
	"testing"
)

func int64SliceToBytesV1(slice ...int64) []byte {
	bz := make([]byte, 0, len(slice)*8)
	for _, ph := range slice {
		buf := make([]byte, 8)
		binary.BigEndian.PutUint64(buf, uint64(ph))
		bz = append(bz, buf...)
	}
	return bz
}

func int64SliceToBytesV2(slice ...int64) []byte {
	bz := make([]byte, len(slice)*8)
	for i, ph := range slice {
		binary.BigEndian.PutUint64(bz[i<<3:], uint64(ph))
	}
	return bz
}

func TestInt64SliceToBytesEquivalence(t *testing.T) {
	testCases := [][]int64{
		{},
		{0},
		{1, 2, 3},
		{-1, 123456789, -987654321},
		{9223372036854775807, -9223372036854775808}, // int64 最大/最小值
	}

	for _, tc := range testCases {
		got1 := int64SliceToBytesV1(tc...)
		got2 := int64SliceToBytesV2(tc...)

		if !bytes.Equal(got1, got2) {
			t.Errorf("Mismatch for input %v:\nV1=%v\nV2=%v", tc, got1, got2)
		}
	}
}

and this test pass.

@cuiweixie
Copy link
Contributor Author

@aljo242 can you help with the gosec check failed. I do not understand the failed log.

@aljo242
Copy link
Contributor

aljo242 commented Oct 1, 2025

Can you unit test this and add a benchmark function for this?

@cuiweixie
Copy link
Contributor Author

cuiweixie commented Oct 1, 2025

Can you unit test this and add a benchmark function for this?

done @aljo242

@aljo242 aljo242 requested a review from technicallyty October 6, 2025 17:17
@aljo242
Copy link
Contributor

aljo242 commented Oct 6, 2025

@technicallyty for the final approval here

@cuiweixie
Copy link
Contributor Author

@aljo242 Done

@technicallyty technicallyty changed the title refactor: less alloc perf: less alloc Oct 7, 2025
@cuiweixie
Copy link
Contributor Author

gentle ping!

Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@aljo242 aljo242 enabled auto-merge October 15, 2025 21:26
@aljo242 aljo242 added this pull request to the merge queue Oct 15, 2025
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.88%. Comparing base (abc3054) to head (8f76647).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #25360   +/-   ##
=======================================
  Coverage   52.87%   52.88%           
=======================================
  Files         797      797           
  Lines       64903    64903           
=======================================
+ Hits        34318    34321    +3     
+ Misses      30585    30582    -3     

see 2 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Merged via the queue into cosmos:main with commit ce8bed2 Oct 15, 2025
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants