Skip to content

Conversation

@mikeprosserni
Copy link
Collaborator

What does this Pull Request accomplish?

I discovered a performance problem when using panel.get_value(value_id, default). If the value_id was not found on the server, the server was throwing an RPC exception to indicate the failure. The client would then catch that exception and return the default value. But throwing an exception across the gRPC channel is quite resource-intensive. It's fine for actually exceptional circumstances, but severely slows things down if the measurement script is using panel.get_value() in a tight loop.

It's easy to imagine users writing scripts that rely on default values, especially during development, so I think this needs to be performant.

The server-side changes can be seen here.

On the client side here, we now use if response.HasField("value"): to check if the server returned a value to us.

Why should this Pull Request be merged?

This is a significant performance improvement.

What testing has been done?

I added a performance checking example, which includes measuring the time it takes for get_value() to handle an unset value:

image

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2025

Test Results

   10 files  ±0     10 suites  ±0   18s ⏱️ -1s
  190 tests ±0    190 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 850 runs  ±0  1 850 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 2f936dc. ± Comparison against base commit db935ae.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
tests.unit.test_panel_client ‑ test___get_unset_value_raises_exception
tests.unit.test_panel_client ‑ test___get_unset_value___returns_not_found

♻️ This comment has been updated with latest results.

@mikeprosserni mikeprosserni marked this pull request as ready for review July 8, 2025 20:13
Mike Prosser added 2 commits July 8, 2025 15:18
@mikeprosserni mikeprosserni merged commit 2f3724a into main Jul 10, 2025
14 checks passed
@mikeprosserni mikeprosserni deleted the users/mprosser/unset-performance branch July 10, 2025 18:07
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.

4 participants