Manual instrumentation #773
Conversation
|
💚 |
… where the runtime block the JavaScript thread Log all c++ frames, log some key methods in Java Add package.json "profiling": "timeline" option to enable the logging of the manual instrumentation profiling
PanayotCankov
force-pushed
the
display-blocking-js
branch
from
225be73 to
5ef6e8d
Compare
|
💚 |
| StaticConfiguration config = new StaticConfiguration(logger, appName, nativeLibDir, rootDir, | ||
| appDir, classLoader, dexDir, dexThumb, appConfig, isDebuggable); | ||
| AppConfig appConfig = new AppConfig(appDir); | ||
| switch (appConfig.getProfilingMode()) { |
There was a problem hiding this comment.
The following lines concerning instrumentation can be moved to the ManualInstrumentation class in a method you call from RuntimeHelper
| // Created by Panayot Cankov on 26/05/2017. | ||
| // | ||
|
|
||
| #ifndef TEST_APP_MANUALINSTRUMENTATION_H |
There was a problem hiding this comment.
Remove the TEST_APP_ prefix
| #include "ManualInstrumentation.h" | ||
|
|
||
| bool tns::instrumentation::Frame::disabled = true; | ||
| const std::chrono::steady_clock::time_point tns::instrumentation::Frame::disabled_time = std::chrono::steady_clock::now(); |
There was a problem hiding this comment.
std::chrono::steady_clock::time_point(); - the empty constructor defaults to duration::zero which should be good enough for a "disabled" value.
| } | ||
|
|
||
| void MetadataNode::ExtendedClassConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>& info) { | ||
| TNSPERF(); |
There was a problem hiding this comment.
What's the difference between using the TNSPERF macro and tns::instrumentation::Frame constructor? Can we make it consistent across the runtime code, and use just one of the two?
There was a problem hiding this comment.
TNSPERF uses __func__ for name and is nothing but a shorthand for tns::instrumentation::Frame frame(__func__);
| } | ||
|
|
||
| string moduleName = ArgConverter::ConvertToString(args[0].As<String>()); | ||
| string frameName("RequireCallback " + moduleName); |
There was a problem hiding this comment.
Can this be inlined with the constructor on the following line? Or rename the frameName variable accordingly so that it becomes clear the RequireCallback moduleName string isn't part of the require logic
There was a problem hiding this comment.
tns::instrumentation::Frame frame(("RequireCallback " + moduleName).c_str());
There was a problem hiding this comment.
string("RequireCallback " + moduleName).c_str() for readability
| auto cls = fromJsInfo->ObjectClazz; | ||
| JEnv env; | ||
| jclass clsClazz = env.GetObjectClass(cls); | ||
| jmethodID methodId = env.GetMethodID(clsClazz, "getSimpleName", "()Ljava/lang/String;"); |
There was a problem hiding this comment.
consider caching the jclass and jmethodID in static variables in the Frame class if the frame.check() passes (profiling is enabled)
There was a problem hiding this comment.
I think we have GET_NAME_METHOD_ID handy at scope, I will reuse it instead.
There was a problem hiding this comment.
Along with m_env instead of new JEnv env?
There was a problem hiding this comment.
if (frame.check()) {
auto cls = fromJsInfo->ObjectClazz;
jclass clsClazz = m_env.GetObjectClass(cls);
jstring className = (jstring) m_env.CallObjectMethod(cls, GET_NAME_METHOD_ID);
frame.log(("MarkReachableObjects: " + ArgConverter::jstringToString(className)).c_str());
}
There was a problem hiding this comment.
probably frame.log needs a string& override
There was a problem hiding this comment.
That sounds like a good idea, it will call internally to frame.log(char *)
There was a problem hiding this comment.
same for the constructor
| jclass clsClazz = env.GetObjectClass(cls); | ||
| jmethodID methodId = env.GetMethodID(clsClazz, "getSimpleName", "()Ljava/lang/String;"); | ||
| jstring className = (jstring) env.CallObjectMethod(cls, methodId); | ||
| const char* str = env.GetStringUTFChars(className, NULL); |
There was a problem hiding this comment.
Lines 527, 528, 530 can be replaced with the following
auto jsObjClassName = "MarkReachableObjects: " + ArgConverter::jstringToString(className);
|
@Pip3r4o most of the comments have been addressed (except the macro I think) could you take a look on the changes after the second commit? |
|
💚 |
|
Good job! |
3 participants
Setting in app/package.json a:
Will enable tracing of slow manually instrumented methods.
The facility can trace both java and cpp frames, there is already a matching implementation in the modules. The complete feature lets us profile and trace times in cpp java and js in a single timeline.