perf(core): StringId hold bytes to avoid decode/encode #2862

javeme · 2025-08-30T17:22:08Z

We can avoid the decode of temporary IDs now. Temporary IDs are not ultimately returned to the user and do not require JSON serialization, for example, the intermediate vertex ids of the adjacent edge query.

Once binary serialization is supported, we can also directly return bytes serialization to the user.

Change-Id: I1ff93a2ee31f142fb7ab5cfe0e038ef2da2f982d

codecov · 2025-08-30T17:33:22Z

Codecov Report

❌ Patch coverage is 62.50000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.00%. Comparing base (0946d5d) to head (d705eac).

Files with missing lines	Patch %	Lines
...a/org/apache/hugegraph/backend/id/IdGenerator.java	71.42%	2 Missing and 6 partials ⚠️
...graph/memory/consumer/impl/id/StringIdOffHeap.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2862      +/-   ##
============================================
+ Coverage     40.91%   41.00%   +0.09%     
  Complexity      333      333              
============================================
  Files           747      747              
  Lines         60168    60189      +21     
  Branches       7683     7692       +9     
============================================
+ Hits          24615    24679      +64     
+ Misses        32975    32923      -52     
- Partials       2578     2587       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

imbajin · 2025-09-01T08:35:41Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

@@ -128,14 +129,19 @@ public static int compareType(Id id1, Id id2) {
    public static class StringId implements Id {

        protected String id;
+        protected byte[] bytes;


Thread Safety Concern: The lazy initialization pattern used here is not thread-safe. If multiple threads access the same StringId instance simultaneously, race conditions could occur during the lazy initialization of id and bytes fields. Consider using volatile fields or synchronization to ensure thread safety.

At present, I haven't found any scenarios where this would cause problems. It's safe even if multiple assignments occur at the same time.

imbajin · 2025-09-01T08:36:07Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

@@ -128,14 +129,19 @@ public static int compareType(Id id1, Id id2) {
    public static class StringId implements Id {

        protected String id;
+        protected byte[] bytes;

        public StringId(String id) {


Inconsistent State Risk: The constructor initializes bytes = null explicitly, but the state where both id and bytes are null is not handled. Consider adding validation to ensure at least one field is non-null, or document the invariant that exactly one field should be null at any time.

id is null is not allowed in this constructor

imbajin · 2025-09-01T08:36:32Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

+            if (this.id != null) {
+                return this.id.compareTo(other.asString());
+            } else {
+                return Bytes.compare(this.bytes, other.asBytes());


Performance Optimization: Consider using Arrays.equals() instead of Bytes.equals() for byte array comparison if Bytes.equals() is just a wrapper. Also, cache the result of other.asBytes() to avoid multiple conversions in the comparison.

imbajin · 2025-09-01T08:36:53Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

+            StringId other = (StringId) obj;
+            if (this.id != null) {
+                return this.id.equals(other.asString());
+            } else {


Potential Memory Leak: In the equals method, calling other.asBytes() when this.id != null may trigger unnecessary byte array creation and caching in the other object. Consider checking if the other object has the same representation type first to avoid forced conversions.

imbajin · 2025-09-01T08:37:11Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

@@ -128,14 +129,19 @@ public static int compareType(Id id1, Id id2) {
    public static class StringId implements Id {


Missing Test Coverage: This performance optimization changes core behavior of StringId. Please ensure comprehensive test coverage including:\n- Thread safety tests with concurrent access\n- Edge cases with empty/null inputs\n- Performance regression tests\n- Memory usage validation tests

Copilot

Pull Request Overview

This PR optimizes the StringId class to hold both string and byte representations to avoid unnecessary decode/encode operations. The change improves performance by lazily initializing either the string or byte representation depending on how the StringId is constructed, eliminating redundant conversions.

Key changes:

Added byte array field to StringId to store raw bytes alongside string representation
Modified constructors to initialize only one representation (string or bytes) depending on input
Updated methods to lazily decode/encode only when needed

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
IdGenerator.java	Added bytes field to StringId class and implemented lazy encoding/decoding logic
StringIdOffHeap.java	Added cleanup of bytes field in memory release method

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

Copilot · 2025-09-01T08:43:39Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

+            if (this.bytes == null) {
+                this.bytes = StringEncoding.encode(this.id);
+            }
+            return this.bytes;


Returning the internal byte array directly allows external modification of the StringId's state. Consider returning a defensive copy with return this.bytes.clone(); to maintain immutability.

Suggested change

return this.bytes;

return this.bytes.clone();

Copilot · 2025-09-01T08:43:39Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

+                return this.id.compareTo(other.asString());
+            } else {
+                return Bytes.compare(this.bytes, other.asBytes());


This optimization to avoid string conversion during comparison may not be effective since other.asBytes() will still trigger encoding if the other StringId only has a string representation. Consider checking if both objects have bytes available before using byte comparison.

Copilot · 2025-09-01T08:43:39Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

                return false;
            }
-            return this.id.equals(((StringId) other).id);
+            StringId other = (StringId) obj;
+            if (this.id != null) {


Similar to the compareTo method, this optimization may not be effective since other.asString() and other.asBytes() will trigger conversions if the other StringId doesn't have the required representation. Consider optimizing for cases where both objects have the same representation type.

javeme · 2025-09-07T12:23:21Z

...gegraph-core/src/main/java/org/apache/hugegraph/memory/consumer/impl/id/StringIdOffHeap.java

@@ -70,6 +70,7 @@ public void serializeSelfToByteBuf(MemoryPool memoryPool) {
    @Override
    public void releaseOriginalVarsOnHeap() {
        this.id = null;
+        this.bytes = null;


in serializeSelfToByteBuf we can set stringBytes = this.bytes if this.bytes != null

javeme · 2025-09-07T12:39:14Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

@@ -128,14 +129,19 @@ public static int compareType(Id id1, Id id2) {
    public static class StringId implements Id {

        protected String id;
+        protected byte[] bytes;


At present, I haven't found any scenarios where this would cause problems. It's safe even if multiple assignments occur at the same time.

javeme · 2025-09-07T12:40:02Z

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java

@@ -128,14 +129,19 @@ public static int compareType(Id id1, Id id2) {
    public static class StringId implements Id {

        protected String id;
+        protected byte[] bytes;

        public StringId(String id) {


id is null is not allowed in this constructor

Change-Id: I436858dbad61ecc2e83861b05a6620a46b6df245

perf(core): StringId hold bytes to avoid decode/encode

7202d54

Change-Id: I1ff93a2ee31f142fb7ab5cfe0e038ef2da2f982d

dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Aug 30, 2025

javeme requested review from z7658329, Pengzna and imbajin August 30, 2025 17:22

dosubot bot added the perf label Aug 30, 2025

imbajin reviewed Sep 1, 2025

View reviewed changes

imbajin requested a review from Copilot September 1, 2025 08:42

Copilot AI reviewed Sep 1, 2025

View reviewed changes

javeme commented Sep 7, 2025

View reviewed changes

fix comments

d705eac

Change-Id: I436858dbad61ecc2e83861b05a6620a46b6df245

		@@ -128,14 +129,19 @@ public static int compareType(Id id1, Id id2) {
		public static class StringId implements Id {

perf(core): StringId hold bytes to avoid decode/encode #2862

Are you sure you want to change the base?

perf(core): StringId hold bytes to avoid decode/encode #2862

Conversation

javeme commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

javeme commented Aug 30, 2025 •

edited

Loading

codecov bot commented Aug 30, 2025 •

edited

Loading