Skip to content

Memory and time improvements - #1358

Merged
louis-e merged 3 commits into
mainfrom
memory-time-improvements
Sep 27, 2026
Merged

Memory and time improvements#1358
louis-e merged 3 commits into
mainfrom
memory-time-improvements

Conversation

@louis-e

@louis-e louis-e commented Sep 27, 2026

Copy link
Copy Markdown
Owner

No description provided.

Copilot AI lite review requested due to automatic review settings September 27, 2026 11:08
Copilot AI reviewed Sep 27, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Critical cache-retention and temporary-file collision issues remain unresolved, along with moderate performance and metric concerns.

Review effort: Lite
Findings: 2 High severity

Open (2)
What changed in this PR

Improves memory usage and execution speed across terrain, OSM, schematic, palette, and tile-generation paths.

Changes:

  • Interns block properties and caches palette color conversions.
  • Adds OSM filtering, canopy row caching, water memoization, and terrain shortcuts.
  • Reworks tile processing into a bounded concurrent pipeline.
File Summary Findings
src/​structures/​schematic.rs Reuses rotated block properties. Moderate: cache lookup is linear in distinct property states.
src/​osm_tiles.rs Filters and orders tile-derived OSM elements. —
src/​landmarks.rs Interns rotated landmark properties. —
src/​ground_generation.rs Memoizes nearby water checks. —
src/​elevation/​postprocess.rs Adds terrain and mask-processing shortcuts. —
src/​element_processing/​railways.rs Deduplicates tunnel carving. —
src/​data_processing.rs Pipelines tile workers with ordered merging. Moderate: placement timing includes merge and eviction work.
src/​colors.rs Restricts a test-only helper. —
src/​caves/​schems.rs Interns rotated cave properties. —
src/​caves/​decoration.rs Interns decoration properties. —
src/​canopy/​mod.rs Adds persistent row-block caching. Critical: temporary filenames can collide during concurrent fetches.
src/​block_palette.rs Caches palette Oklab values. —
src/​block_definitions.rs Adds block-state interning. Critical: process-lifetime interning retains values without bounds.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/block_definitions.rs Outdated
Comment thread src/canopy/mod.rs Outdated
@github-actions

github-actions Bot commented Sep 27, 2026 •
edited
Loading

Copy link
Copy Markdown

⏱️ Benchmark run finished in 0m 25s
🏗️ Generation time: 17s (excl. data fetching)
🧠 Peak memory usage: 1079 MB

📈 Compared against baseline: 17s time, 1068 MB memory
🧮 Delta: 0s time, 11 MB memory
🔢 Commit: 1b344df

🟢 Generation time is unchanged.
🟢 Peak memory is unchanged.

📅 Last benchmark: 2026-09-27 11:36:48 UTC

You can retrigger the benchmark by commenting retrigger-benchmark.

@louis-e
louis-e requested a balanced review from Copilot September 27, 2026 11:34
Copilot AI reviewed Sep 27, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The new tile pipeline can oversubscribe CPUs and reports pipeline duration as placement time.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Nested Rayon Work Oversubscribes CPU Beyond Configured Thread Cap

src/​data_processing.rs:1198

These are dedicated OS workers in addition to the Rayon pool, but process_tile still enters Rayon-parallel cave work (caves::noise_cells and, under eviction, seal_floating_fluid_region). With caves enabled, tile workers at other stages can run concurrently with all Rayon workers, exceeding the configured CPU-thread cap and causing oversubscription. Keep tile scheduling on the Rayon pool or make the nested per-tile work serial while this outer pool is active.

Medium severity Placement Metric Incorrectly Includes Pipeline and Merge Time

src/​data_processing.rs:1338

place_start begins before the scoped pipeline, so this duration includes ordered merging, region flushing, channel waits, and placement. Reporting it as element_placement makes this metric incomparable with the previous placement-only measurement and also attributes the separately reported merge time to placement. Rename it to a pipeline wall-time metric, or instrument placement separately.

@louis-e
louis-e merged commit 788d5e9 into main Sep 27, 2026
4 checks passed
@louis-e
louis-e deleted the memory-time-improvements branch September 27, 2026 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@louis-e