Skip to content

⚡ Offload Gem.setGems to async task#17

Open
acsoto wants to merge 1 commit intomainfrom
perf/gem-command-async-14764446331830774432
Open

⚡ Offload Gem.setGems to async task#17
acsoto wants to merge 1 commit intomainfrom
perf/gem-command-async-14764446331830774432

Conversation

@acsoto
Copy link
Contributor

@acsoto acsoto commented Feb 1, 2026

💡 What:

  • Wrapped the blocking GemExecutor.setGems call in CommandGem.set with BukkitRunnable.runTaskAsynchronously.
  • Refactored CommandGem.set() to accept CommandSender and args as parameters instead of relying on instance fields, ensuring thread safety for the async task.
  • Added CommandGemBenchmarkTest to verify the performance improvement and correct async execution.
  • Updated pom.xml to include JUnit, Mockito, and ByteBuddy for testing, and fixed the PlaceholderAPI repository/version to ensure build success.

🎯 Why:

  • The database update was running on the main thread, causing potential server freezes (blocking ~100ms in simulation).
  • Relying on this.sender in an async task (or even generally in a singleton CommandExecutor) is unsafe if concurrent commands were possible or if the state changed before async execution.

📊 Measured Improvement:

  • Baseline: ~105ms (simulated blocking delay).
  • After: ~5ms (non-blocking).
  • Verified that the task is correctly scheduled and the logic inside executes as expected using ArgumentCaptor in tests.

PR created automatically by Jules for task 14764446331830774432 started by @acsoto

Co-authored-by: acsoto <59144459+acsoto@users.noreply.github.com>
@google-labs-jules
Copy link

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copilot AI review requested due to automatic review settings February 1, 2026 16:25
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8d9ad6cb6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +70 to +75
new BukkitRunnable() {
@Override
public void run() {
Gem.getPlugin().getGemExecutor().setGems(playerName, gems);
sender.sendMessage(Message.INFO + playerName + " 的宝石设置为: " + gems);
}

Choose a reason for hiding this comment

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

P2 Badge Avoid calling Bukkit API from async thread

This async task now calls sender.sendMessage(...) from the scheduler’s async thread. In Bukkit/Spigot, most API calls (including messaging a CommandSender/Player) are not thread-safe and are expected to run on the main server thread. If sender is a player, this can surface as warnings or intermittent issues under load. The async work should be limited to the database call, then schedule a sync task to send the message back on the main thread.

Useful? React with 👍 / 👎.

Copy link

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 offloads the blocking GemExecutor.setGems() database operation to an async task to prevent main thread blocking during gem management operations. The change wraps the database call in BukkitRunnable.runTaskAsynchronously() and refactors the method signature to accept parameters instead of using instance fields for thread safety.

Changes:

  • Modified CommandGem.set() to execute database operations asynchronously using BukkitRunnable
  • Refactored set() method to accept CommandSender and args as parameters for thread safety
  • Added test infrastructure with JUnit, Mockito, and ByteBuddy dependencies and a benchmark test to verify async behavior
  • Updated PlaceholderAPI version from 2.11.1 to 2.11.5

Reviewed changes

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

File Description
modules/Gem/src/main/java/com/mcatk/gem/command/CommandGem.java Modified set() method to wrap database operation in async task and accept parameters instead of using instance fields
modules/Gem/src/test/java/com/mcatk/gem/command/CommandGemBenchmarkTest.java Added test to verify async execution and performance improvement with mocked Bukkit environment
modules/Gem/pom.xml Added test dependencies (JUnit, Mockito, ByteBuddy) and updated PlaceholderAPI version

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

switch (args[0].toLowerCase()) {
case "set":
set();
set(sender, args);
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The instance fields this.sender and this.args are still being set in onCommand() (lines 26-27), even though the set() method has been refactored to use parameters instead. This creates an incomplete fix: while set() is now thread-safe, the other methods (check(), add(), take(), total(), printHelp(), sendParameterError()) still rely on these instance fields. If multiple commands are executed concurrently or if async tasks complete after new commands arrive, these methods could read incorrect values. Consider either: (1) removing these instance fields entirely and passing parameters to all methods, or (2) making all database-accessing methods async with parameter passing like set() was changed.

Copilot uses AI. Check for mistakes.
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>5.2.0</version>
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

There is a version mismatch between mockito-core (5.11.0) and mockito-inline (5.2.0). These should typically use the same version to avoid potential compatibility issues. Consider updating mockito-inline to 5.11.0 to match mockito-core, or verify that this specific version combination is intentional and compatible.

Suggested change
<version>5.2.0</version>
<version>5.11.0</version>

Copilot uses AI. Check for mistakes.
import static org.mockito.Mockito.*;

@RunWith(MockitoJUnitRunner.class)
public class CommandGemBenchmarkTest {
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The class is named "CommandGemBenchmarkTest" but it's actually a functional test with a timing assertion rather than a true benchmark. Benchmarks typically involve multiple iterations, warmup phases, and statistical analysis. Consider renaming to "CommandGemAsyncTest" or similar to better reflect its purpose, or use a proper benchmarking framework like JMH if benchmarking is the actual goal.

Suggested change
public class CommandGemBenchmarkTest {
public class CommandGemAsyncTest {

Copilot uses AI. Check for mistakes.
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.

1 participant