Skip to content

Remove weak callback from __postFrameCallback cache #1755

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
May 16, 2023
Merged

Remove weak callback from __postFrameCallback cache #1755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2a91657
feat: Remove NDK profiler
ptomato Apr 7, 2023
8a38dc6
chore: Remove outdated comment
ptomato Apr 7, 2023
357535d
chore: Use unique_ptr for NewBackingStore in byte buffers
ptomato Apr 7, 2023
c7b89c9
chore: Remove weak finalizer callback from __postFrameCallback()
ptomato Apr 7, 2023
c8bd1dc
fix: Make sure frame callbacks are not ignored
ptomato Apr 7, 2023
57ea7b3
chore: Clear callback caches' persistent handles in destructor
ptomato Apr 7, 2023
f0d5e93
chore: Construct FrameCallbackCacheEntry with ID
ptomato Apr 7, 2023
b0a1190
chore: Improve usage of unordered_map APIs in CallbackHandlers
ptomato Apr 7, 2023
05c7897
chore: Fix runtime typo
ptomato Apr 7, 2023
bca974b
chore: Ignore main thread and frame callback return values
ptomato Apr 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions test-app/runtime/src/main/cpp/ArrayBufferHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,13 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo<Value>
auto data = env.GetDirectBufferAddress(obj);
auto size = env.GetDirectBufferCapacity(obj);

std::shared_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
std::unique_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
data,
size,
[](void* data, size_t length, void* deleter_data) {},
data);
BackingStore::EmptyDeleter,
nullptr);

arrayBuffer = ArrayBuffer::New(isolate, store);
arrayBuffer = ArrayBuffer::New(isolate, std::move(store));
} else {
if (m_remainingMethodID == nullptr) {
m_remainingMethodID = env.GetMethodID(m_ByteBufferClass, "remaining", "()I");
Expand All @@ -117,15 +117,15 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo<Value>
jbyte* data = new jbyte[bufferRemainingSize];
memcpy(data, byteArrayElements, bufferRemainingSize);

std::shared_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
std::unique_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
data,
bufferRemainingSize,
[](void *data, size_t length, void *deleter_data) {
delete[] ((jbyte*)data);
},
nullptr);

arrayBuffer = ArrayBuffer::New(isolate, store);
arrayBuffer = ArrayBuffer::New(isolate, std::move(store));
env.ReleaseByteArrayElements(byteArray, byteArrayElements, 0);
}

Expand Down
75 changes: 22 additions & 53 deletions test-app/runtime/src/main/cpp/CallbackHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,8 +671,11 @@ void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo<v8::Va

uint64_t key = ++count_;
Local<v8::Function> callback = args[0].As<v8::Function>();
CacheEntry entry(isolate, callback, context);
cache_.emplace(key, std::move(entry));

bool inserted;
std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback, context);
assert(inserted && "Main thread callback ID should not be duplicated");

auto value = Callback(key);
auto size = sizeof(Callback);
auto wrote = write(Runtime::GetWriter(),&value , size);
Expand All @@ -697,32 +700,20 @@ int CallbackHandlers::RunOnMainThreadFdCallback(int fd, int events, void *data)
auto context = it->second.context_.Get(isolate);
Context::Scope context_scope(context);
Local<v8::Function> cb = it->second.callback_.Get(isolate);
Local<Value> result;

v8::TryCatch tc(isolate);

if (!cb->Call(context, context->Global(), 0, nullptr).ToLocal(&result)) {}
cb->Call(context, context->Global(), 0, nullptr); // ignore JS return value

cache_.erase(it);

if(tc.HasCaught()){
throw NativeScriptException(tc);
}

RemoveKey(key);

return 1;
}

void CallbackHandlers::RemoveKey(const uint64_t key) {
auto it = cache_.find(key);
if (it == cache_.end()) {
return;
}

it->second.callback_.Reset();
it->second.context_.Reset();
cache_.erase(it);
}

void CallbackHandlers::LogMethodCallback(const v8::FunctionCallbackInfo<v8::Value> &args) {
try {
if ((args.Length() > 0) && args[0]->IsString()) {
Expand Down Expand Up @@ -834,10 +825,10 @@ void CallbackHandlers::EnableVerboseLoggingMethodCallback(
const v8::FunctionCallbackInfo<v8::Value> &args) {
try {
auto isolate = args.GetIsolate();
auto runtume = Runtime::GetRuntime(isolate);
auto runtime = Runtime::GetRuntime(isolate);
tns::LogEnabled = true;
JEnv env;
env.CallVoidMethod(runtume->GetJavaRuntime(), ENABLE_VERBOSE_LOGGING_METHOD_ID);
env.CallVoidMethod(runtime->GetJavaRuntime(), ENABLE_VERBOSE_LOGGING_METHOD_ID);
} catch (NativeScriptException &e) {
e.ReThrowToV8();
} catch (std::exception e) {
Expand All @@ -855,10 +846,10 @@ void CallbackHandlers::DisableVerboseLoggingMethodCallback(
const v8::FunctionCallbackInfo<v8::Value> &args) {
try {
auto isolate = args.GetIsolate();
auto runtume = Runtime::GetRuntime(isolate);
auto runtime = Runtime::GetRuntime(isolate);
tns::LogEnabled = false;
JEnv env;
env.CallVoidMethod(runtume->GetJavaRuntime(), DISABLE_VERBOSE_LOGGING_METHOD_ID);
env.CallVoidMethod(runtime->GetJavaRuntime(), DISABLE_VERBOSE_LOGGING_METHOD_ID);
} catch (NativeScriptException &e) {
e.ReThrowToV8();
} catch (std::exception e) {
Expand Down Expand Up @@ -1589,16 +1580,12 @@ void CallbackHandlers::TerminateWorkerThread(Isolate *isolate) {
void CallbackHandlers::RemoveIsolateEntries(v8::Isolate *isolate) {
for (auto &item: cache_) {
if (item.second.isolate_ == isolate) {
item.second.callback_.Reset();
item.second.context_.Reset();
cache_.erase(item.first);
}
}

for (auto &item: frameCallbackCache_) {
if (item.second.isolate_ == isolate) {
item.second.callback_.Reset();
item.second.context_.Reset();
frameCallbackCache_.erase(item.first);
}
}
Expand Down Expand Up @@ -1653,12 +1640,10 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &

if (success && pId->IsNumber()){
auto id = pId->IntegerValue(context).FromMaybe(0);
if(frameCallbackCache_.contains(id)){
auto cb = frameCallbackCache_.find(id);
if(cb != frameCallbackCache_.end()){
cb->second.removed = false;
PostCallback(args, &cb->second, context);
}
auto cb = frameCallbackCache_.find(id);
if (cb != frameCallbackCache_.end()) {
cb->second.removed = false;
PostCallback(args, &cb->second, context);
return;
}
}
Expand All @@ -1668,26 +1653,10 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &

V8SetPrivateValue(isolate, func, idKey, v8::Number::New(isolate, (double) key));

FrameCallbackCacheEntry entry (isolate, callback, context);
entry.id = key;

auto finalCallback = [](const v8::WeakCallbackInfo<FrameCallbackCacheEntry> &data) {
auto value = data.GetParameter();
for (auto &item: frameCallbackCache_) {
if (item.second.id == value->id) {
item.second.context_.Reset();
item.second.callback_.Reset();
frameCallbackCache_.erase(item.first);
break;
}
}
};

entry.callback_.SetWeak(&entry, finalCallback, v8::WeakCallbackType::kFinalizer);

frameCallbackCache_.emplace(key, std::move(entry));

auto val = frameCallbackCache_.find(key);
robin_hood::unordered_map<uint64_t, FrameCallbackCacheEntry>::iterator val;
bool inserted;
std::tie(val, inserted) = frameCallbackCache_.try_emplace(key, isolate, callback, context, key);
assert(inserted && "Frame callback ID should not be duplicated");

PostCallback(args, &val->second, context);
}
Expand Down Expand Up @@ -1715,8 +1684,8 @@ void CallbackHandlers::RemoveFrameCallback(const FunctionCallbackInfo<v8::Value>

if (success && pId->IsNumber()){
auto id = pId->IntegerValue(context).FromMaybe(0);
if(frameCallbackCache_.contains(id)){
auto cb = frameCallbackCache_.find(id);
auto cb = frameCallbackCache_.find(id);
if (cb != frameCallbackCache_.end()) {
cb->second.removed = true;
}
}
Expand Down
34 changes: 21 additions & 13 deletions test-app/runtime/src/main/cpp/CallbackHandlers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,6 @@ namespace tns {
jobject _runtime;
};

static void RemoveKey(const uint64_t key);

static std::atomic_int64_t count_;


Expand All @@ -305,6 +303,11 @@ namespace tns {
context_(isolate, context){
}

~CacheEntry() {
context_.Reset();
callback_.Reset();
}

v8::Isolate* isolate_;
v8::Global<v8::Function> callback_;
v8::Global<v8::Context> context_;
Expand All @@ -316,17 +319,23 @@ namespace tns {
static std::atomic_uint64_t frameCallbackCount_;

struct FrameCallbackCacheEntry {
FrameCallbackCacheEntry(v8::Isolate *isolate, v8::Local<v8::Function> callback, v8::Local<v8::Context> context)
: isolate_(isolate),
callback_(isolate,callback),
context_(isolate, context)
{
FrameCallbackCacheEntry(v8::Isolate *isolate, v8::Local<v8::Function> callback,
v8::Local<v8::Context> context, uint64_t aId)
: isolate_(isolate),
callback_(isolate, callback),
context_(isolate, context),
id(aId) {
}

~FrameCallbackCacheEntry() {
context_.Reset();
callback_.Reset();
}

v8::Isolate *isolate_;
v8::Global<v8::Function> callback_;
v8::Global<v8::Context> context_;
bool removed;
bool removed = false;
uint64_t id;

AChoreographer_frameCallback frameCallback_ = [](long ts, void *data) {
Expand All @@ -341,6 +350,7 @@ namespace tns {
if (data != nullptr) {
auto entry = static_cast<FrameCallbackCacheEntry *>(data);
if (entry->removed) {
frameCallbackCache_.erase(entry->id); // invalidates *entry
return;
}
v8::Isolate *isolate = entry->isolate_;
Expand All @@ -352,15 +362,13 @@ namespace tns {
v8::Context::Scope context_scope(context);

v8::Local<v8::Function> cb = entry->callback_.Get(isolate);
v8::Local<v8::Value> result;

v8::Local<v8::Value> args[1] = {v8::Number::New(isolate, ts)};

v8::TryCatch tc(isolate);

if (!cb->Call(context, context->Global(), 1, args).ToLocal(&result)) {
// TODO
}
cb->Call(context, context->Global(), 1, args); // ignore JS return value

frameCallbackCache_.erase(entry->id); // invalidates *entry

if(tc.HasCaught()){
throw NativeScriptException(tc);
Expand Down
1 change: 0 additions & 1 deletion test-app/runtime/src/main/cpp/MessageLoopTimer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ int MessageLoopTimer::PumpMessageLoopCallback(int fd, int events, void* data) {
v8::HandleScope handleScope(isolate);

while (v8::platform::PumpMessageLoop(Runtime::platform, isolate)) {
// isolate->RunMicrotasks();
isolate->PerformMicrotaskCheckpoint();
}

Expand Down
52 changes: 0 additions & 52 deletions test-app/runtime/src/main/cpp/Profiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ void Profiler::Init(Isolate* isolate, const Local<Object>& globalObj, const stri
globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__startCPUProfiler"), FunctionTemplate::New(isolate, Profiler::StartCPUProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__stopCPUProfiler"), FunctionTemplate::New(isolate, Profiler::StopCPUProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__heapSnapshot"), FunctionTemplate::New(isolate, Profiler::HeapSnapshotMethodCallback, extData)->GetFunction(context).ToLocalChecked());
globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__startNDKProfiler"), FunctionTemplate::New(isolate, Profiler::StartNDKProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__stopNDKProfiler"), FunctionTemplate::New(isolate, Profiler::StopNDKProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
}

void Profiler::StartCPUProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {
Expand Down Expand Up @@ -195,56 +193,6 @@ bool Profiler::Write(CpuProfile* cpuProfile) {
return true;
}

void Profiler::StartNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {
try {
auto isolate = args.GetIsolate();
auto extData = args.Data().As<External>();
auto thiz = static_cast<Profiler*>(extData->Value());
thiz->StartNDKProfiler();
} catch (NativeScriptException& e) {
e.ReThrowToV8();
} catch (std::exception e) {
stringstream ss;
ss << "Error: c++ exception: " << e.what() << endl;
NativeScriptException nsEx(ss.str());
nsEx.ReThrowToV8();
} catch (...) {
NativeScriptException nsEx(std::string("Error: c++ exception!"));
nsEx.ReThrowToV8();
}
}

void Profiler::StopNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {
try {
auto isolate = args.GetIsolate();
auto extData = args.Data().As<External>();
auto thiz = static_cast<Profiler*>(extData->Value());
thiz->StopNDKProfiler();
} catch (NativeScriptException& e) {
e.ReThrowToV8();
} catch (std::exception e) {
stringstream ss;
ss << "Error: c++ exception: " << e.what() << endl;
NativeScriptException nsEx(ss.str());
nsEx.ReThrowToV8();
} catch (...) {
NativeScriptException nsEx(std::string("Error: c++ exception!"));
nsEx.ReThrowToV8();
}
}

void Profiler::StartNDKProfiler() {
#ifdef NDK_PROFILER_ENABLED
monstartup("libNativeScript.so");
#endif
}

void Profiler::StopNDKProfiler() {
#ifdef NDK_PROFILER_ENABLED
moncleanup();
#endif
}

class FileOutputStream: public OutputStream {
public:
FileOutputStream(FILE* stream) :
Expand Down
8 changes: 0 additions & 8 deletions test-app/runtime/src/main/cpp/Profiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ class Profiler {

static void StopCPUProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args);

static void StartNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args);

static void StopNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args);

static void HeapSnapshotMethodCallback(const v8::FunctionCallbackInfo<v8::Value>& args);

void StartCPUProfilerCallbackImpl(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand All @@ -33,10 +29,6 @@ class Profiler {

bool StopCPUProfiler(v8::Isolate* isolate, const v8::Local<v8::String>& name);

void StartNDKProfiler();

void StopNDKProfiler();

bool Write(v8::CpuProfile* cpuProfile);

std::string m_appName;
Expand Down