Skip to content

cache currentTimeMillis - #344

Merged
tr7zw merged 2 commits into
tr7zw:mainfrom
Wyvest:main
Oct 12, 2026
Merged

tr7zw merged 2 commits into
tr7zw:mainfrom
Wyvest:main

Conversation

@Wyvest

@Wyvest Wyvest commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

On macOS (idk if this is the same elsewhere), running System.currentTimeMillis takes on average 14.7 ns compared to a normal field read (~0.3 ns). With 2,000 culled entities running at 500fps this would be ~1.5% of frame time.

This PR moves the check to run after cullable.isCulled and caches currentTimeMillis so that this method call has little to no effect on frame time.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Walkthrough

The change adds a shared timestamp updated at each client tick and uses it for forced-visibility timeouts. Entity and block-entity render paths now check culling before forced visibility.

Changes

Visibility and culling

Layer / File(s) Summary
Client-tick forced-visibility timeout
src/main/java/dev/tr7zw/entityculling/EntityCullingModBase.java, src/main/java/dev/tr7zw/entityculling/mixin/CullableMixin.java
EntityCullingModBase initializes and updates tickTimeMs at client ticks. CullableMixin uses that timestamp to set and check the forced-visibility timeout.
Render-path culling checks
src/main/java/dev/tr7zw/entityculling/mixin/WorldRendererMixin.java, src/main/java/dev/tr7zw/entityculling/mixin/BlockEntityRenderDispatcherMixin.java
Entity and block-entity render paths check isCulled() before isForcedVisible(). The other culling conditions remain unchanged.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: tr7zw

Merge Risk: ⚪ Minimal · up to 09064

The reviewed changes have no demonstrated user-facing regression in the inspected render and paused-client paths. The repository does not enforce the optional Sonar warning, so no known issue blocks merging.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: caching System.currentTimeMillis through a shared tick-time field.
Description check Passed The description directly explains the performance motivation and the two implementation changes: checking culling before forced visibility and caching the time value.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔇 Additional comments (5)
src/main/java/dev/tr7zw/entityculling/EntityCullingModBase.java (1)

39-39: LGTM!

src/main/java/dev/tr7zw/entityculling/mixin/CullableMixin.java (1)

8-8: LGTM!

Also applies to: 35-35, 40-40

src/main/java/dev/tr7zw/entityculling/mixin/WorldRendererMixin.java (1)

54-54: LGTM!

Also applies to: 161-161

src/main/java/dev/tr7zw/entityculling/mixin/BlockEntityRenderDispatcherMixin.java (1)

77-77: LGTM!

Also applies to: 116-116

src/main/java/dev/tr7zw/entityculling/EntityCullingModBase.java-81-81 (1)

81-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Move the static write into a static helper.

The SonarCloud diagnostic flags this assignment. clientTick() uses instance state, so it cannot be static. Move the assignment into a static helper and call that helper here.

Proposed change
     public void clientTick() {
-        tickTimeMs = System.currentTimeMillis();
+        updateTickTime();

Add a static helper:

private static void updateTickTime() {
    tickTimeMs = System.currentTimeMillis();
}

Source: Linters/SAST tools


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cf4ef110-9b0a-4efc-9a06-8327ee602d48
📥 Commits

Reviewing files that changed from the base of the PR and between 5269dd5 and 09064e4.

📒 Files selected for processing (4)
  • src/main/java/dev/tr7zw/entityculling/EntityCullingModBase.java
  • src/main/java/dev/tr7zw/entityculling/mixin/BlockEntityRenderDispatcherMixin.java
  • src/main/java/dev/tr7zw/entityculling/mixin/CullableMixin.java
  • src/main/java/dev/tr7zw/entityculling/mixin/WorldRendererMixin.java

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

@tr7zw
tr7zw merged commit 8fef2d0 into tr7zw:main Oct 12, 2026
3 of 4 checks passed
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