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:02:26 UTC
[PR] perf(core): optimize perf by avoid boxing long [incubator-hugegraph]
javeme opened a new pull request, #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861
- Query: E.checkArgument avoid boxing long
- BytesBuffer: E.checkArgument avoid Bytes.toHex and boxing long
- define const RANGE_TYPES to avoid construct every 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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#discussion_r2313272466
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Query.java:
##########
@@ -287,17 +293,27 @@ public long total() {
public long limit() {
if (this.capacity != NO_CAPACITY) {
Review Comment:
**Code style inconsistency**: While the performance optimization is good, this pattern creates inconsistent code style throughout the file. Consider applying the same refactoring pattern to all similar validation methods for consistency, or use a helper method approach for better maintainability.
--
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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies merged PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861
--
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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#discussion_r2313296090
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/serializer/BytesBuffer.java:
##########
@@ -162,16 +165,21 @@ public int remaining() {
private void require(int size) {
// Does need to resize?
- if (this.buffer.limit() - this.buffer.position() >= size) {
+ if (this.buffer.remaining() >= size) {
return;
}
// Can't resize for wrapped buffer since will change the origin ref
- E.checkState(this.resize, "Can't resize for wrapped buffer");
+ if (!this.resize) {
+ E.checkState(false, "Can't resize for wrapped buffer");
+ }
Review Comment:
[nitpick] The pattern `E.checkState(false, ...)` is less readable than the original `E.checkState(condition, ...)`. Consider using `E.checkState(this.resize, ...)` instead to maintain code clarity.
```suggestion
E.checkState(this.resize, "Can't resize for wrapped buffer");
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Query.java:
##########
@@ -287,17 +293,27 @@ public long total() {
public long limit() {
if (this.capacity != NO_CAPACITY) {
- E.checkArgument(this.limit == Query.NO_LIMIT ||
- this.limit <= this.capacity,
- "Invalid limit %s, must be <= capacity(%s)",
- this.limit, this.capacity);
+ if (this.limit == NO_LIMIT) {
+ /*
+ * TODO: may need to return this.capacity here.
+ * In most cases, capacity == DEFAULT_CAPACITY, then limit()
+ * will return NO_LIMIT, thus rely on Query.checkCapacity()
+ * to check in the next step.
+ */
+ }
Review Comment:
Empty if-block with only a TODO comment creates dead code path. Either implement the logic mentioned in the TODO or remove this unnecessary conditional block.
```suggestion
// TODO: may need to return this.capacity here.
// In most cases, capacity == DEFAULT_CAPACITY, then limit()
// will return NO_LIMIT, thus rely on Query.checkCapacity()
// to check in the next step.
```
--
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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#discussion_r2313269445
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/serializer/BytesBuffer.java:
##########
@@ -249,7 +257,7 @@ public byte peek() {
}
public byte peekLast() {
- return this.buffer.get(this.buffer.capacity() - 1);
+ return this.buffer.get(this.buffer.limit() - 1);
Review Comment:
**Critical bug fix**: This change from `buffer.capacity() - 1` to `buffer.limit() - 1` is correct and important. Using capacity would access potentially uninitialized data beyond the actual buffer content.
--
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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#issuecomment-3239406730
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2861?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 93.25%. 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 ([`1e1cf2e`](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/1e1cf2e0a3fa9c5bb818beccadf0b23ab943d75c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2861 +/- ##
=============================================
+ Coverage 40.91% 93.25% +52.34%
+ Complexity 333 65 -268
=============================================
Files 747 9 -738
Lines 60168 267 -59901
Branches 7683 22 -7661
=============================================
- Hits 24615 249 -24366
+ Misses 32975 8 -32967
+ Partials 2578 10 -2568
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2861?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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#discussion_r2313270548
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Condition.java:
##########
@@ -137,6 +137,12 @@ public enum RelationType implements BiPredicate<Object, Object> {
return true;
});
+ private static final Set<RelationType> RANGE_TYPES =
Review Comment:
**Performance optimization**: Excellent refactoring! Moving these constant sets to static final fields avoids recreating the same ImmutableSet instances on every method call. This is a significant performance improvement for frequently called methods.
--
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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#discussion_r2313268217
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/serializer/BytesBuffer.java:
##########
@@ -162,16 +165,21 @@ public int remaining() {
private void require(int size) {
// Does need to resize?
- if (this.buffer.limit() - this.buffer.position() >= size) {
+ if (this.buffer.remaining() >= size) {
Review Comment:
**Bug fix**: Good catch! Using `buffer.remaining()` is more efficient and semantically correct than manually calculating `buffer.limit() - buffer.position()`.
--
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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#discussion_r2328620335
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Query.java:
##########
@@ -180,7 +180,9 @@ public long offset() {
}
public void offset(long offset) {
- E.checkArgument(offset >= 0L, "Invalid offset %s", offset);
+ if (offset < 0L) {
Review Comment:
due to there is no a universal check condition, but adding methods for various check conditions is not a good idea.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Query.java:
##########
@@ -287,17 +293,27 @@ public long total() {
public long limit() {
if (this.capacity != NO_CAPACITY) {
Review Comment:
We only modify the high-frequency codes on the main path and keep the other low-frequency codes in their original style.
--
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): optimize perf by avoid boxing long [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2861:
URL: https://github.com/apache/incubator-hugegraph/pull/2861#discussion_r2313265321
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Query.java:
##########
@@ -180,7 +180,9 @@ public long offset() {
}
public void offset(long offset) {
- E.checkArgument(offset >= 0L, "Invalid offset %s", offset);
+ if (offset < 0L) {
Review Comment:
**Performance optimization concern**: While avoiding autoboxing is good, this pattern is harder to read and maintain. Consider using a static helper method instead:
```java
private static void checkNonNegative(long value, String message, Object... args) {
if (value < 0L) {
E.checkArgument(false, message, args);
}
}
```
Then call: `checkNonNegative(offset, "Invalid offset %s", offset);`
--
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