Details

Reviewers

• jon
tomek
• max
varun

Commits

rCOMM4486a9c39522: [services] Rust Integration - c++ - Add getNumberOfCores

Summary

Depends on D5038

We need a thread pool to address the grpc's threading problem in c++. To decide what can be a limit for the threads in the thread pool, we should probably obtain a number of CPUs on the machine.

Test Plan

services build as usual

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Sep 12 2022, 7:10 AM

Herald added a reviewer: • jon. · View Herald TranscriptSep 12 2022, 7:10 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

• karol added a child revision: D5068: [services] Rust Integration - c++ - Add worker.Sep 12 2022, 7:11 AM

• karol edited the summary of this revision. (Show Details)

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, • max, varun.

• karol added a parent revision: D5038: [services] Rust Integration - Backup - c++ - Multiple get clients.

Harbormaster completed remote builds in B12082: Diff 16583.Sep 12 2022, 7:20 AM

• karol requested review of this revision.Sep 12 2022, 7:20 AM

• jon accepted this revision.Sep 12 2022, 9:09 AM

This revision is now accepted and ready to land.Sep 12 2022, 9:09 AM

tomek requested changes to this revision.Sep 13 2022, 11:01 AM

tomek added inline comments.

services/lib/src/GlobalTools.cpp
32 ↗	(On Diff #16583)	This name is confusing: for the majority of devices the number of CPUs is simply 1. What you probably wanted to express here is the number of cores. I guess that due to features like multi-threading, the number of cores doesn't necessarily equals number of hardware threads, but still that name is a lot less misleading. Returns the number of concurrent threads supported by the implementation.
33 ↗	(On Diff #16583)	The docs https://en.cppreference.com/w/cpp/thread/thread/hardware_concurrency specify that the return value is Number of concurrent threads supported. If the value is not well defined or not computable, returns 0. But we're not handling the `0` in any way.

This revision now requires changes to proceed.Sep 13 2022, 11:01 AM

Thanks for the review. Addressed feedback.

Harbormaster completed remote builds in B12131: Diff 16653.Sep 14 2022, 1:54 AM

tomek accepted this revision.Sep 15 2022, 7:30 AM

tomek added inline comments.

services/lib/src/GlobalTools.cpp
34 ↗	(On Diff #16653)	Have you checked if `u` suffix works?

This revision is now accepted and ready to land.Sep 15 2022, 7:30 AM

• karol retitled this revision from [services] Rust Integration - c++ - Add getNumberOfCpus to [services] Rust Integration - c++ - Add getNumberOfCores.Sep 16 2022, 1:47 AM

• karol added inline comments.Sep 16 2022, 1:49 AM