Skip to content

Reduce memory churn by not calling shrink_to_fit#338

Open
greenscientist wants to merge 1 commit into
chairemobilite:v2cfrom
greenscientist:noshrink_to_fit
Open

Reduce memory churn by not calling shrink_to_fit#338
greenscientist wants to merge 1 commit into
chairemobilite:v2cfrom
greenscientist:noshrink_to_fit

Conversation

@greenscientist
Copy link
Copy Markdown
Contributor

@greenscientist greenscientist commented Mar 4, 2026

On an high thread count, we are getting contention on memory by constantly allocating and deallocating memory. We can save some of that by not calling shrink_to_fit. We might use a bit more memory, but gain on performance

Summary by CodeRabbit

  • Refactor
    • Adjusted memory management strategy to reduce explicit memory capacity releases during data resets and cache operations.

On an high thread count, we are getting contention on memory by
constantly allocating and deallocating memory. We can save some
of that by not calling shrink_to_fit. We might use a bit more performance, but gain on performance
@greenscientist greenscientist requested a review from tahini March 4, 2026 21:58
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 4, 2026

Walkthrough

The PR removes three calls to shrink_to_fit() across three source files. In resets.cpp, two calls are removed from accessFootpaths and egressFootpaths after clearing. In transit_data.cpp, two calls are removed from forwardConnections and reverseConnections after populating. In trips_and_connections_cache_fetcher.cpp, one call is removed from the connections vector after clearing. No control flow, error handling, or logic is altered; only explicit memory capacity reduction is affected.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing shrink_to_fit() calls to reduce memory churn, which aligns with all modifications across the three files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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 and usage tips.

@greenscientist
Copy link
Copy Markdown
Contributor Author

We might need to benchmark this a bit more

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

@tahini
Copy link
Copy Markdown
Collaborator

tahini commented Mar 5, 2026

As-tu roulé les benchmarks sur cette PR?

@greenscientist
Copy link
Copy Markdown
Contributor Author

As-tu roulé les benchmarks sur cette PR?

Il faut que j'en refasse, maintenant qu'on a identifié le bottleneck to dispatch http.

Copy link
Copy Markdown
Collaborator

@tahini tahini left a comment

Choose a reason for hiding this comment

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

Putting a request changes to not merge accidentally until we know what the benchmarks say and confirm it is needed

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.

3 participants