-
-
Notifications
You must be signed in to change notification settings - Fork 363
Fix: Energy reporting should report energy not time #5768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6a5ecbf
to
2bdbafa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5768 +/- ##
=============================================
+ Coverage 86.568% 86.703% +0.135%
=============================================
Files 423 423
Lines 36310 36333 +23
Branches 15473 17103 +1630
=============================================
+ Hits 31433 31502 +69
+ Misses 4837 4550 -287
- Partials 40 281 +241
... and 49 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @noahsmartin.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bce9765 | 1229.42 ms | 1243.49 ms | 14.07 ms |
8ad303c | 1220.02 ms | 1231.79 ms | 11.77 ms |
e64d3d4 | 1241.90 ms | 1260.10 ms | 18.20 ms |
42cfd79 | 1222.13 ms | 1244.23 ms | 22.10 ms |
9e0030e | 1222.78 ms | 1242.23 ms | 19.45 ms |
5aa3ce5 | 1222.78 ms | 1245.39 ms | 22.61 ms |
f92cfa9 | 1217.94 ms | 1240.06 ms | 22.12 ms |
63ac649 | 1192.10 ms | 1216.78 ms | 24.68 ms |
efab7d3 | 1219.98 ms | 1252.12 ms | 32.14 ms |
d3f650a | 1225.45 ms | 1241.86 ms | 16.41 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bce9765 | 23.74 KiB | 874.06 KiB | 850.32 KiB |
8ad303c | 23.75 KiB | 879.24 KiB | 855.49 KiB |
e64d3d4 | 23.75 KiB | 855.37 KiB | 831.62 KiB |
42cfd79 | 23.75 KiB | 880.20 KiB | 856.45 KiB |
9e0030e | 23.75 KiB | 893.72 KiB | 869.97 KiB |
5aa3ce5 | 23.75 KiB | 904.54 KiB | 880.79 KiB |
f92cfa9 | 23.75 KiB | 855.38 KiB | 831.63 KiB |
63ac649 | 23.75 KiB | 855.38 KiB | 831.63 KiB |
efab7d3 | 23.75 KiB | 912.78 KiB | 889.03 KiB |
d3f650a | 23.75 KiB | 902.48 KiB | 878.73 KiB |
ebd0fca
to
26e51b1
Compare
26e51b1
to
a61aad4
Compare
The data being reported here was CPU time, not energy (in joules) despite the API request claiming it was reporting joules. This led to invalid data being displayed on the frontend, the UI would say it was graphing watts, but it wasn't actually. You could see this by writing CPU intensive code (I used a simd operation) on a userInitiated queue and comparing it to the same operation on a background queue. Here's the result from my test app before this change on a userInitiatedQueue:
and on a background queue:

The shape is the same, and neither show reasonable numbers for watts (< 0.1W). The shape is the same because about the same amount of CPU time is used, one was just expected to draw more power.
With this fix the graph looks much better and you can clearly see the difference that a userInitiated and background queue make on energy consumption:
