Implement metric for app start time #5213
Conversation
briantting
force-pushed
the
time
branch
2 times, most recently
from
8906f66 to
75013d6
Compare
| @@ -2,6 +2,9 @@ | |||
| [ | |||
| ImplementedAs=PerformanceExtensions | |||
| ] partial interface Performance { | |||
| // Returns the application startup duration (us) in bytes, according to | |||
| // base::SysInfo::AppStartupTime(). | |||
| [CallWith=ScriptState] unsigned long long getAppStartupTime(); | |||
There was a problem hiding this comment.
Unless absolutely needed, we should try to use asynchronous Promises (https://www.w3.org/TR/design-principles/#one-time-events). If this API is kept for legacy compatibility reasons, ideally we'd quote that here as e.g. H5vccMetrics does in https://source.corp.google.com/h/lbshell-internal/cobalt_src/+/main:third_party/blink/renderer/modules/cobalt/h5vcc_metrics/h_5_vcc_metrics.idl;l=15-17?q=h5vcc_metrics.idl&ss=h%2Flbshell-internal%2Fcobalt_src%2F%2B%2Frefs%2Fheads%2Fmain -- @hlwarriner and @andrewsavage1 FYI.
There was a problem hiding this comment.
I think we can do async here, but let me check with Sergei
Edit: regardless, changing it between async and sync is easy, so no need to pause on implementing the rest of this PR and saving this decision for the end
There was a problem hiding this comment.
Talked to Sergei and Niketh and they're fine with it being async so we should make it async. @briantting lmk if you need me to walk you through how to do that, but there are a number of h5vcc examples already for you to reference for the implementation
third_party/blink/renderer/core/cobalt/performance/performance_extensions.cc
Outdated
Show resolved
Hide resolved
|
@briantting I noticed this is still in draft mode so haven't yet left feedback - but please lmk if I should go ahead and review. |
briantting
force-pushed
the
time
branch
4 times, most recently
from
a18a06d to
e5abdda
Compare
Implements the plumbing for app startup time metric and calls base::SysInfo::AppStartupTime() whose value can be retrieved devtools by calling performance.GetAppStartupTime(). b/389132127
base/system/sys_info.cc
Outdated
|
|
||
| using starboard::android::shared::StarboardBridge; | ||
| #elif BUILDFLAG(IS_COBALT) | ||
| #include "cobalt/cobalt.h" |
There was a problem hiding this comment.
This breaks layering in a little awkward way. Cobalt is expected to depend on //base, but //base isn't expected to depend on Cobalt.
//base depending on //starboard is okay though.
There's maybe two ways to avoid this :
-
Capture the timestamp for the non-android case also somewhere in Starboard code
-
Or invert the dependency here and add a
SysInfo::SetAppStartTimestamp()that can be called from Cobalt
As a stopgap, this here isn't very bad either though, as we are breaking layering in a few other places. @andrewsavage1 wdyt ?
There was a problem hiding this comment.
I'm trying to invert the dependency with the follow up commit in this PR, but having some difficulties.
|
it's a wip and running into linking problems |
Implements the plumbing for app start time metric but calls base::SysInfo::AppStartupTime() as a dummy placeholder whose value can be retrieved through devtools by calling
performance.GetAppStartupTime().
b/389132127