You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "javeme (via GitHub)" <gi...@apache.org> on 2025/08/30 17:22:10 UTC
[PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
javeme opened a new pull request, #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862
Change-Id: I1ff93a2ee31f142fb7ab5cfe0e038ef2da2f982d
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies merged PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#issuecomment-3239419185
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2862?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `78.26087%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 33.81%. Comparing base ([`0946d5d`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/0946d5dd0cad8c27cb365b0c2aaaf217cdca3402?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7202d54`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/7202d54c34d906fd62a8e79b3ad68ae6856ebe4b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2862?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/org/apache/hugegraph/backend/id/IdGenerator.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2862?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Fbackend%2Fid%2FIdGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2JhY2tlbmQvaWQvSWRHZW5lcmF0b3IuamF2YQ==) | 81.81% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2862?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...graph/memory/consumer/impl/id/StringIdOffHeap.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2862?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Fmemory%2Fconsumer%2Fimpl%2Fid%2FStringIdOffHeap.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL21lbW9yeS9jb25zdW1lci9pbXBsL2lkL1N0cmluZ0lkT2ZmSGVhcC5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2862?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (0946d5d) and HEAD (7202d54). Click for more details.
>
> <details><summary>HEAD has 2 uploads less than BASE</summary>
>
>| Flag | BASE (0946d5d) | HEAD (7202d54) |
>|------|------|------|
>||3|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2862 +/- ##
============================================
- Coverage 40.91% 33.81% -7.10%
+ Complexity 333 264 -69
============================================
Files 747 747
Lines 60168 60181 +13
Branches 7683 7688 +5
============================================
- Hits 24615 20350 -4265
- Misses 32975 37535 +4560
+ Partials 2578 2296 -282
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2862?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2313278747
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java:
##########
@@ -174,25 +186,34 @@ public int compareTo(Id other) {
if (cmp != 0) {
return cmp;
}
- return this.id.compareTo(other.asString());
+ if (this.id != null) {
+ return this.id.compareTo(other.asString());
+ } else {
+ return Bytes.compare(this.bytes, other.asBytes());
+ }
}
@Override
public int hashCode() {
- return this.id.hashCode();
+ return this.asString().hashCode();
}
@Override
- public boolean equals(Object other) {
- if (!(other instanceof StringId)) {
+ public boolean equals(Object obj) {
+ if (!(obj instanceof StringId)) {
return false;
}
- return this.id.equals(((StringId) other).id);
+ StringId other = (StringId) obj;
+ if (this.id != null) {
+ return this.id.equals(other.asString());
+ } else {
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2313279379
##########
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 {
Review Comment:
**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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2328678710
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java:
##########
@@ -174,25 +186,34 @@ public int compareTo(Id other) {
if (cmp != 0) {
return cmp;
}
- return this.id.compareTo(other.asString());
+ if (this.id != null) {
+ return this.id.compareTo(other.asString());
+ } else {
+ return Bytes.compare(this.bytes, other.asBytes());
+ }
}
@Override
public int hashCode() {
- return this.id.hashCode();
+ return this.asString().hashCode();
}
@Override
- public boolean equals(Object other) {
- if (!(other instanceof StringId)) {
+ public boolean equals(Object obj) {
+ if (!(obj instanceof StringId)) {
return false;
}
- return this.id.equals(((StringId) other).id);
+ StringId other = (StringId) obj;
+ if (this.id != null) {
+ return this.id.equals(other.asString());
+ } else {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2328667470
##########
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;
Review Comment:
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.
##########
hugegraph-server/hugegraph-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;
Review Comment:
in serializeSelfToByteBuf we can set stringBytes = this.bytes if this.bytes != null
##########
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) {
Review Comment:
id is null is not allowed in this constructor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2313276037
##########
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;
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2313276967
##########
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) {
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2313277928
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java:
##########
@@ -174,25 +186,34 @@ public int compareTo(Id other) {
if (cmp != 0) {
return cmp;
}
- return this.id.compareTo(other.asString());
+ if (this.id != null) {
+ return this.id.compareTo(other.asString());
+ } else {
+ return Bytes.compare(this.bytes, other.asBytes());
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] perf(core): StringId hold bytes to avoid decode/encode [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2862:
URL: https://github.com/apache/incubator-hugegraph/pull/2862#discussion_r2313297393
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java:
##########
@@ -145,27 +151,33 @@ public IdType type() {
@Override
public Object asObject() {
- return this.id;
+ return this.asString();
}
@Override
public String asString() {
+ if (this.id == null) {
+ this.id = StringEncoding.decode(this.bytes);
+ }
return this.id;
}
@Override
public long asLong() {
- return Long.parseLong(this.id);
+ return Long.parseLong(this.asString());
}
@Override
public byte[] asBytes() {
- return StringEncoding.encode(this.id);
+ if (this.bytes == null) {
+ this.bytes = StringEncoding.encode(this.id);
+ }
+ return this.bytes;
Review Comment:
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.
```suggestion
return this.bytes.clone();
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java:
##########
@@ -174,25 +186,34 @@ public int compareTo(Id other) {
if (cmp != 0) {
return cmp;
}
- return this.id.compareTo(other.asString());
+ if (this.id != null) {
+ return this.id.compareTo(other.asString());
+ } else {
+ return Bytes.compare(this.bytes, other.asBytes());
+ }
}
@Override
public int hashCode() {
- return this.id.hashCode();
+ return this.asString().hashCode();
}
@Override
- public boolean equals(Object other) {
- if (!(other instanceof StringId)) {
+ public boolean equals(Object obj) {
+ if (!(obj instanceof StringId)) {
return false;
}
- return this.id.equals(((StringId) other).id);
+ StringId other = (StringId) obj;
+ if (this.id != null) {
Review Comment:
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.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/id/IdGenerator.java:
##########
@@ -174,25 +186,34 @@ public int compareTo(Id other) {
if (cmp != 0) {
return cmp;
}
- return this.id.compareTo(other.asString());
+ if (this.id != null) {
+ return this.id.compareTo(other.asString());
+ } else {
+ return Bytes.compare(this.bytes, other.asBytes());
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org