Skip to content

Avoid SetLongField call when GetPrimitiveArrayCritical return NULL#374

Merged
luben merged 2 commits intoluben:masterfrom
cxzl25:avoid_SetLongField
Feb 4, 2026
Merged

Avoid SetLongField call when GetPrimitiveArrayCritical return NULL#374
luben merged 2 commits intoluben:masterfrom
cxzl25:avoid_SetLongField

Conversation

@cxzl25
Copy link
Copy Markdown
Contributor

@cxzl25 cxzl25 commented Feb 4, 2026

When src_buff == NULL, there is no need to call SetLongField

    void *src_buff = (*env)->GetPrimitiveArrayCritical(env, src, NULL);
    if (src_buff == NULL) goto E2;


E2: (*env)->ReleasePrimitiveArrayCritical(env, dst, dst_buff, 0);
    (*env)->SetLongField(env, obj, src_pos_id, input.pos);
    (*env)->SetLongField(env, obj, dst_pos_id, output.pos);

@luben
Copy link
Copy Markdown
Owner

luben commented Feb 4, 2026

On one side I agree that there is no need to synchronize the buffer positions if no action was taken. On another side memory allocation failure is quite rare and I don't thin the duplication and readability hit is worth the performance gain in that case.

If you still think it's worth it, better approach will be to move the 2 SetLongField before both ReleaseArrayCritical

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.81%. Comparing base (c76455c) to head (87dfbb4).
⚠️ Report is 66 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #374      +/-   ##
============================================
+ Coverage     60.01%   60.81%   +0.79%     
- Complexity      308      334      +26     
============================================
  Files            26       27       +1     
  Lines          1473     1623     +150     
  Branches        170      192      +22     
============================================
+ Hits            884      987     +103     
- Misses          434      465      +31     
- Partials        155      171      +16     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@luben luben merged commit 2d94b35 into luben:master Feb 4, 2026
7 of 8 checks passed
dongjoon-hyun added a commit to apache/spark that referenced this pull request Feb 10, 2026
### What changes were proposed in this pull request?

This PR aims to upgrade `zstd-jni` to 1.5.7-7.

### Why are the changes needed?

To use the latest bug fixes:
- https://github.com/luben/zstd-jni/releases/tag/v1.5.7-7
  - luben/zstd-jni#372
  - luben/zstd-jni#374

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: `Gemini 3 Pro (High)` on `Antigravity`

Closes #54233 from dongjoon-hyun/SPARK-55456.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
rpnkv pushed a commit to rpnkv/spark that referenced this pull request Feb 18, 2026
### What changes were proposed in this pull request?

This PR aims to upgrade `zstd-jni` to 1.5.7-7.

### Why are the changes needed?

To use the latest bug fixes:
- https://github.com/luben/zstd-jni/releases/tag/v1.5.7-7
  - luben/zstd-jni#372
  - luben/zstd-jni#374

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: `Gemini 3 Pro (High)` on `Antigravity`

Closes apache#54233 from dongjoon-hyun/SPARK-55456.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants