Use a permanent thread local Calculation object by greenscientist · Pull Request #363 · chairemobilite/trRouting

greenscientist · 2026-07-28T17:52:13Z

One of the main performance issue we have is the memory allocation churn of creating and delete the Calculation object for each request. Instead of always reallocating for each object, we can use a local object for each thread. Since we only have one request at a time being handled on a thread, we don't have concurrency issues.

We have to call reset at the start of each request to clear out the data.

Summary by CodeRabbit

Performance Improvements
- Improved transit routing request processing by reusing per-thread calculation resources across requests instead of creating new instances each time.
- Added request-level timing so performance metrics capture each request’s calculation duration more accurately.
Bug Fixes
- Improved calculation state cleanup during resets by clearing egress footpaths and access/egress node state earlier, reducing the risk of stale routing data affecting subsequent computations.

coderabbitai · 2026-07-28T17:52:37Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a0ffd361-7100-46cf-9eed-c2e11f57500d

📥 Commits

Reviewing files that changed from the base of the PR and between 1df1bc7 and 89821b7.

📒 Files selected for processing (3)

connection_scan_algorithm/include/calculator.hpp
connection_scan_algorithm/src/resets.cpp
connection_scan_algorithm/src/transit_routing_http_server.cpp

📜 Recent review details

🔇 Additional comments (3)

connection_scan_algorithm/include/calculator.hpp (1)
80-85: 🎯 Functional Correctness

Confirm that start() restarts an active timer.

Because the same Calculator is reused, CalculationTime::start() must reset the start point on every request; otherwise timing logs may accumulate across requests. Please verify the implementation and add a focused regression test.
#!/usr/bin/env bash
set -euo pipefail
rg -n --hidden --glob '!build/**' \
  '\b(class|struct)\s+CalculationTime\b|\bCalculationTime::start\b' .
connection_scan_algorithm/src/resets.cpp (1)

28-37: LGTM!

Also applies to: 75-75, 109-109

connection_scan_algorithm/src/transit_routing_http_server.cpp (1)

360-360: LGTM!

Also applies to: 408-408, 456-456

Walkthrough

The routing server now reuses a thread-local Calculator for route, summary, and accessibility requests. Each request starts the calculator timer, while reset logic consolidates clearing of routing, access, and egress state. An inline startRequestTimer() method was added to Calculator.

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

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: switching to a permanent thread-local calculator object.
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.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

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.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp`:
- Around line 153-169: Update getThreadCalculator and the Calculator cleanup
path to account for retained peak routing-buffer memory: add metrics for
per-thread peak allocation and implement an explicit trimming policy, such as
shrinking buffers beyond a defined threshold after reset/clear. If trimming is
intentionally deferred, document enforceable request-size and compute-thread
limits instead, while preserving thread-local reuse for normal requests.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d3ae1c29-dcc9-4a0a-8aab-cd265668f69f

📥 Commits

Reviewing files that changed from the base of the PR and between 04fed85 and 9e03012.

📒 Files selected for processing (3)

connection_scan_algorithm/include/calculator.hpp
connection_scan_algorithm/src/resets.cpp
connection_scan_algorithm/src/transit_routing_http_server.cpp

📜 Review details

⏰ Context from checks skipped due to timeout. (6)

GitHub Check: pr-build-check
GitHub Check: Test build on macOS-latest (memcached: false)
GitHub Check: Test build on macOS-latest (memcached: true)
GitHub Check: Test build on ubuntu-latest (memcached: false)
GitHub Check: Test build on ubuntu-latest (memcached: true)
GitHub Check: Analyze (cpp)

🧰 Additional context used

🪛 Clang (14.0.6)

connection_scan_algorithm/src/transit_routing_http_server.cpp

[warning] 161-161: use a trailing return type for this function

(modernize-use-trailing-return-type)

🪛 Cppcheck (2.21.0)

connection_scan_algorithm/src/resets.cpp

[style] 36-36: The function 'getArrivalTime' is never used.

(unusedFunction)

[style] 37-37: The function 'getTrip' is never used.

(unusedFunction)

[style] 36-36: The function 'accept' is never used.

(unusedFunction)

🔇 Additional comments (4)

connection_scan_algorithm/src/resets.cpp (2)

75-75: LGTM!

Also applies to: 110-110

31-37: 🗄️ Data Integrity & Integration

No change needed.

Reset flags are only disabled for alternative calculations, with the fastest route calculating fresh, so first requests do not retain prior footpath/filter state.

connection_scan_algorithm/src/transit_routing_http_server.cpp (1)

360-360: LGTM!

Also applies to: 408-408, 456-456

connection_scan_algorithm/include/calculator.hpp (1)

80-86: 🎯 Functional Correctness

No change needed.

CalculationTime::start() sets startEpoch to the current epoch, so startRequestTimer() restarts the request-spanning timer as intended.

github-actions · 2026-07-29T05:06:34Z

@coderabbitai review

coderabbitai · 2026-07-29T05:06:41Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

tahini · 2026-07-29T14:20:52Z

+{
+  thread_local Calculator calculator(transitData, geoFilter);
+  // Start the timer at the beginning of each request.
+  // TODO Maybe we should be it out of the calculator


This sentence does not make sense: "be it out"

Je trouve ca étrange que le lapin aille pas vu ça!

tahini · 2026-07-29T14:25:24Z

+// tasks queued on the compute pool.
+static Calculator & getThreadCalculator(const TransitData &transitData, GeoFilter &geoFilter)
+{
+  thread_local Calculator calculator(transitData, geoFilter);


C'est le fait que la fonction est statique qui fait que la variable définie comme thread_local à l'intérieur existe durant toute la durée du thread? Je ne connais pas ce keyword... La variable n'est pas définie "globalement/par thread", elle est quand même dans cette fonction après tout.

https://en.cppreference.com/cpp/language/storage_duration

Thread storage duration
All variables declared with thread_local have thread storage duration.

The storage for these entities lasts for the duration of the thread in which they are created. There is a distinct object or reference per thread, and use of the declared name refers to the entity associated with the current thread.

Elle est dans la fonction, mais l'objet existe pour toute la durée du thread. C'est un peu comme statique oui, mais pour chaque thread.

Si tu le dis...

One of the main performance issue we have is the memory allocation churn of creating and delete the Calculation object for each request. Instead of always reallocating for each object, we can use a local object for each thread. Since we only have one request at a time being handled on a thread, we don't have concurrency issues. We have to call reset at the start of each request to clear out the data. We also moved the start timer in the request parsing side, instead of doing it the constructor. In local benchmarks we went from calc/sec: 27.37 to calc/sec: 40.4 Duing the genetic algo we went from Batch routing completed: 18973 trips in 33.4s (avg 568.82 calc/sec) to Batch routing completed: 18982 trips in 16.4s (avg 1160.37 calc/sec)

coderabbitai Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread connection_scan_algorithm/src/transit_routing_http_server.cpp

greenscientist force-pushed the reducememorychurn branch from 9e03012 to 1df1bc7 Compare July 28, 2026 18:48

greenscientist requested review from kaligrafy and tahini July 28, 2026 18:49

greenscientist mentioned this pull request Jul 28, 2026

Reduce memory churn by not calling shrink_to_fit #338

Closed

kaligrafy approved these changes Jul 29, 2026

View reviewed changes

tahini reviewed Jul 29, 2026

View reviewed changes

greenscientist force-pushed the reducememorychurn branch from 1df1bc7 to 89821b7 Compare July 29, 2026 14:37

greenscientist requested a review from tahini July 29, 2026 14:38

tahini approved these changes Jul 29, 2026

View reviewed changes

greenscientist merged commit f400315 into chairemobilite:v2c Jul 29, 2026
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use a permanent thread local Calculation object - #363

Use a permanent thread local Calculation object#363
greenscientist merged 1 commit into
chairemobilite:v2cfrom
greenscientist:reducememorychurn

greenscientist commented Jul 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Review ran into problems

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

tahini Jul 29, 2026

Uh oh!

greenscientist Jul 29, 2026

Uh oh!

tahini Jul 29, 2026

Uh oh!

greenscientist Jul 29, 2026

Uh oh!

tahini Jul 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

greenscientist commented Jul 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning)

Review ran into problems

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

coderabbitai Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tahini Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

greenscientist Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

tahini Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

greenscientist Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

tahini Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greenscientist commented Jul 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading