improve long to bytes with benchmarks by NeatGuyCoding · Pull Request #29 · NeatGuyCoding/netty-socketio

NeatGuyCoding · 2025-10-16T10:10:38Z

Summary by CodeRabbit

Bug Fixes
- Fixed improper handling of zero in byte conversion logic.
Tests
- Added comprehensive benchmark suite to compare conversion implementations and track performance.
Chores
- Optimized byte conversion performance by eliminating logarithm-based calculations.
- Minor code cleanup and import reorganization.

Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>

coderabbitai · 2025-10-16T10:10:50Z

Walkthrough

The changes introduce a performance optimization to the longToBytes method in PacketEncoder, replacing the previous implementation with a zero-safe algorithm that avoids Math.log10. Supporting test infrastructure adds JMH benchmark dependencies and a new benchmark suite to compare implementations. Existing tests are updated to validate the corrected zero-handling behavior.

Changes

Cohort / File(s)	Summary
Build Configuration `netty-socketio-core/pom.xml`	Added two JMH test dependencies: `org.openjdk.jmh:jmh-core:1.37` and `org.openjdk.jmh:jmh-generator-annprocess:1.37` for benchmark support.
Core Optimization `netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketEncoder.java`	Replaced `longToBytes` implementation with zero-safe, log10-free digit conversion algorithm that iteratively computes digit count and fills result array via modulo and division operations.
Performance Testing `netty-socketio-core/src/test/java/com/corundumstudio/socketio/benchmark/LongToBytesBenchmark.java`, `netty-socketio-core/src/test/java/com/corundumstudio/socketio/protocol/PacketEncoderTest.java`	Added JMH benchmark suite comparing original and optimized `longToBytes` implementations; updated zero-handling test to assert correct behavior.
Minor Cleanup `netty-socketio-smoke-test/src/main/java/com/corundumstudio/socketio/smoketest/PerformanceTestRunner.java`, `netty-socketio-spring-boot-starter/src/main/java/com/corundumstudio/socketio/spring/boot/starter/config/NettySocketIOSslConfigProperties.java`	Import reorganization and removal of unused import; no functional changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The core algorithm change in PacketEncoder.longToBytes requires verification of correctness and performance characteristics. The new JMH benchmark file introduces standard annotation-driven setup that is straightforward to validate. Test updates confirm the fix for zero-handling. Minor import changes impose negligible review burden.

Poem

🐰 A long-to-byte conversion hops through the code,
No log10 log-jam down this faster road,
With benchmarks to prove each digit's true fate,
The zero now handles with algorithmic grace,
Performance blooms in this optimized space! 🚀

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% 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 pull request title "improve long to bytes with benchmarks" accurately and concisely captures the main objectives of the changeset. The title directly reflects two key aspects: the optimization of the longToBytes method in PacketEncoder.java (moving from Math.log10 to an iterative digit-counting approach) and the addition of comprehensive JMH benchmarks (LongToBytesBenchmark.java) to measure the performance improvement. The title is clear, specific, and not verbose—it avoids vague terms and noise while providing sufficient context for a teammate to understand the primary change at a glance. The other minor changes (import reorganization and unused import removal) are ancillary to the main purpose and don't contradict the title.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feature-improve-long-to-bytes

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.}

Copilot

Pull Request Overview

This PR improves the longToBytes method in PacketEncoder by fixing a bug with zero values and optimizing performance, while adding comprehensive benchmarking capabilities to measure the improvements.

Fixed the zero-value bug that caused NegativeArraySizeException when Math.log10(0) returned negative infinity
Replaced Math.log10 with iterative length calculation for better performance
Added JMH benchmark infrastructure and comprehensive performance testing

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
PacketEncoder.java	Fixed zero-value bug and optimized longToBytes method implementation
PacketEncoderTest.java	Updated test to verify zero-value fix works correctly
LongToBytesBenchmark.java	Added JMH benchmark class for performance comparison
pom.xml	Added JMH dependencies for benchmark testing
PerformanceTestRunner.java	Reorganized imports for better code organization
NettySocketIOSslConfigProperties.java	Removed unused import

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/benchmark/LongToBytesBenchmark.java (1)
42-61: Clarify the "original" implementation comment.

The longToBytesOriginal method comment (line 44) states "This is the old version that had issues with zero values," but the implementation includes a zero-handling fix (lines 48-50). While this is appropriate for a fair performance comparison, the comment could be more precise.

Consider rewording to:
/**
 * Original Math.log10-based implementation for performance comparison.
 * Zero handling has been added to ensure a fair benchmark comparison.
 */

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 79c542a and a18b40b.

📒 Files selected for processing (6)

netty-socketio-core/pom.xml (1 hunks)
netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketEncoder.java (1 hunks)
netty-socketio-core/src/test/java/com/corundumstudio/socketio/benchmark/LongToBytesBenchmark.java (1 hunks)
netty-socketio-core/src/test/java/com/corundumstudio/socketio/protocol/PacketEncoderTest.java (1 hunks)
netty-socketio-smoke-test/src/main/java/com/corundumstudio/socketio/smoketest/PerformanceTestRunner.java (1 hunks)
netty-socketio-spring-boot-starter/src/main/java/com/corundumstudio/socketio/spring/boot/starter/config/NettySocketIOSslConfigProperties.java (0 hunks)

💤 Files with no reviewable changes (1)

netty-socketio-spring-boot-starter/src/main/java/com/corundumstudio/socketio/spring/boot/starter/config/NettySocketIOSslConfigProperties.java

🧰 Additional context used

🧬 Code graph analysis (2)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/protocol/PacketEncoderTest.java (1)

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketEncoder.java (1)

PacketEncoder (33-399)

netty-socketio-core/src/test/java/com/corundumstudio/socketio/benchmark/LongToBytesBenchmark.java (1)

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketEncoder.java (1)

PacketEncoder (33-399)

⏰ 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). (2)

GitHub Check: build (17) / build
GitHub Check: build (21) / build

🔇 Additional comments (5)

netty-socketio-smoke-test/src/main/java/com/corundumstudio/socketio/smoketest/PerformanceTestRunner.java (1)

27-33: LGTM! Import reorganization only.

The changes are limited to import statement reorganization with no functional impact.

netty-socketio-core/pom.xml (1)

133-144: LGTM! JMH dependencies added correctly.

The JMH 1.37 dependencies (core and annotation processor) are properly configured with test scope to support the new benchmark infrastructure. Version 1.37 is current and stable.

netty-socketio-core/src/test/java/com/corundumstudio/socketio/protocol/PacketEncoderTest.java (1)

548-553: LGTM! Test correctly validates zero handling.

The test now properly validates that longToBytes(0L) returns a single-element array containing the numeric byte 0, which aligns with the corrected zero-safe implementation.

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketEncoder.java (1)

235-259: LGTM! Clean zero-safe implementation.

The revised longToBytes implementation correctly:

Handles the zero case explicitly (returns {0})

Calculates length via division loop, avoiding Math.log10 for better performance

Extracts digits in proper order using modulo and division

The approach is straightforward and eliminates the previous zero-handling gap.

netty-socketio-core/src/test/java/com/corundumstudio/socketio/benchmark/LongToBytesBenchmark.java (1)

31-82: LGTM! Well-structured JMH benchmark.

The benchmark configuration and setup are sound:

Standard JMH annotations with appropriate warmup/measurement iterations

Good parameter coverage including edge cases (zero, single digit, large values)

Clean separation between optimized and original implementations

Main method provides convenient execution entry point

improve long to bytes with benchmarks

a18b40b

Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>

Copilot AI review requested due to automatic review settings October 16, 2025 10:10

Copilot AI reviewed Oct 16, 2025

View reviewed changes

coderabbitai bot reviewed Oct 16, 2025

View reviewed changes

NeatGuyCoding merged commit 1ef5f53 into main Oct 16, 2025
3 checks passed

NeatGuyCoding deleted the feature-improve-long-to-bytes branch October 16, 2025 11:24

coderabbitai bot mentioned this pull request Oct 17, 2025

modify long to bytes with benchmarks #30

Merged

coderabbitai bot mentioned this pull request Oct 24, 2025

Upgrade netty version #33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve long to bytes with benchmarks#29

improve long to bytes with benchmarks#29
NeatGuyCoding merged 1 commit intomainfrom
feature-improve-long-to-bytes

NeatGuyCoding commented Oct 16, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

NeatGuyCoding commented Oct 16, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NeatGuyCoding commented Oct 16, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 16, 2025 •

edited

Loading