Skip to content

Conversation

@JasirVoriya
Copy link
Contributor

Issue Title
Performance: avoid sorting when computing compactionDelayTime; use min timestamp

Description
In CompactionManager.start()’s periodic monitor task, compactionDelayTime is computed by sorting server objects by committedTimestamp and then taking the first element. This sorting is unnecessary because we only need the minimum timestamp. Sorting costs O(n log n) time; finding the minimum is O(n). With large object lists, the sort introduces avoidable CPU overhead and potential GC pressure.

Affected Code

  • Path: s3stream/src/main/java/com/automq/stream/s3/compact/CompactionManager.java
  • Method: start()
  • Current logic:
data.sort(Comparator.comparingLong(S3ObjectMetadata::committedTimestamp));
this.compactionDelayTime = System.currentTimeMillis() - data.get(0).committedTimestamp();

Benefits

  • Reduce time complexity from O(n log n) to O(n).
  • Lower CPU usage and reduce GC pressure from sorting.
  • Improve efficiency of the periodic monitoring task in high-object-count scenarios.

Behavior

  • Semantics unchanged: the delay is still based on the earliest committedTimestamp.
  • Edge cases unchanged: when data is null/empty or an exception occurs, delay is set to 0.

Validation

  • Build s3stream module: ./gradlew :s3stream:compileJava -x test should pass.
  • Run regular builds/integration tests to confirm no functional regressions.
  • Optional: profile CPU/time of the monitor task with large object lists to confirm reduction.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2025

CLA assistant check
All committers have signed the CLA.

@Gezi-lzq Gezi-lzq requested a review from Copilot November 3, 2025 09:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR optimizes the computation of compactionDelayTime by replacing a sort-based approach with a single-pass minimum finding algorithm. This improves performance when calculating the delay time metric for compaction monitoring.

Key Changes:

  • Replaces List.sort() followed by get(0) with a manual iteration to find the minimum committedTimestamp
  • Eliminates unnecessary sorting overhead when only the minimum value is needed

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Gezi-lzq
Copy link
Contributor

Gezi-lzq commented Nov 3, 2025

Thank you for your contribution @JasirVoriya . It looks fine overall, but I noticed there are some CI errors that could be resolved.

Prevent delay calculation from using Long.MAX_VALUE; return early.
Also remove unused import.
@JasirVoriya
Copy link
Contributor Author

Thank you for your contribution @JasirVoriya . It looks fine overall, but I noticed there are some CI errors that could be resolved.

Fixed it, thanks for the heads-up @Gezi-lzq .

Copy link
Contributor

@Gezi-lzq Gezi-lzq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@woshigaopp woshigaopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@Mixficsol Mixficsol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@superhx superhx merged commit 9594642 into AutoMQ:main Nov 4, 2025
6 checks passed
Gezi-lzq pushed a commit that referenced this pull request Nov 4, 2025
Gezi-lzq added a commit that referenced this pull request Nov 4, 2025
…of sorting (#2984) (#2989)

Co-authored-by: JasirVoriya <jasirvoriya@gmail.com>
@JasirVoriya JasirVoriya deleted the voriyajasir/compaction-delay-min-fix branch November 10, 2025 07:26
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.

6 participants