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.

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

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Apr 7, 2023

Description

In preparation for upgrading V8 to 11.x, we need to remove uses of kFinalizer weak callbacks because V8 no longer supports them. One was used to clean up the cache entries for __postFrameCallback() callbacks, but that isn't necessary; we can just do the cleanup after the callback runs.

Also fixes a bug where sometimes a frame callback never got called.

Includes various cleanups in CallbackHanders.cpp and CallbackHanders.h, as well as a few misc commits left over from some of the upgrade work I did in February (the first 3 commits in the stack).

Does your pull request have unit tests?

__postFrameCallback() doesn't have a unit test. (If one is needed, let me know and I can write one.)

I tested this by adding the following snippet to the test-app:

--- a/test-app/app/src/main/assets/app/MyActivity.js
+++ b/test-app/app/src/main/assets/app/MyActivity.js
@@ -56,6 +56,13 @@ var MyActivity = (function (_super) {
                        onClick: function () {
                                button.setBackgroundColor(colors[taps % colors.length]);
                                taps++;
+                               textView.setText('Waiting for frame callback...');
+                               __postFrameCallback(() => {
+                                   textView.setText(`Message after one second: ${new Date()}`);
+                               }, 1000 /* ms */);
+                               __runOnMainThread(() => {
+                                   button.setText(`Hit me ${5 - taps} more times`);
+                               });
                        },
                })
         );

ptomato added 9 commits April 6, 2023 17:17
BREAKING CHANGE: __startNDKProfiler() and __stopNDKProfiler() global
bindings no longer available.

The NDK profiler was not functional, since nothing in the build process
defined the NDK_PROFILER_ENABLED preprocessor symbol. The start and stop
functions were already no-ops.
RunMicrotasks no longer exists, it's already been replaced in the code
with PerformMicrotaskCheckpoint.
V8 doc: https://docs.google.com/document/d/1sTc_jRL87Fu175Holm5SV0kajkseGl2r8ifGY76G35k/edit

The V8 usage examples show unique_ptr here; it probably doesn't matter
much, but we don't need the backing store after creating the ArrayBuffer,
and I believe shared_ptr is slightly more overhead than unique_ptr.

For convenience, replace the manual empty deleter for direct buffers with
v8::BackingStore::EmptyDeleter.
Weak finalizer callbacks are going away in V8 11.x, so we have to remove
this one. Luckily, a finalizer callback is not necessary - it's never
needed to prevent the frame callback from being collected.

However, a weak callback is not necessary in the first place. We can just
clean up the cache entry after the callback is executed, since it is only
executed once.

Note that the call to erase() destructs the FrameCallbackCacheEntry
instance, making `entry` a dangling pointer, so don't use it after the
erase(). There's probably a safer way to do this, although the way that
first occurred to me (pass the key to the callback instead of the entry,
and then use std::unordered_map::extract()) is not available on
robin_hood::unordered_map.
There was a bug where __postFrameCallback() would not always cause the
callback to be called. Without initializing `removed`, sometimes it would
have a nonzero value, so the callback would be ignored.
Clearing the persistent handles in the destructor makes it a bit easier to
deal with the cache entry's lifetime: they are cleared whenever the entry
is removed from the cache.

We do this for both the main thread callback cache and the frame callback
cache.

Adding a destructor makes the cache entries non-movable. But the only
place where they were moved was when inserting them into the cache anyway.
We can use C++17's try_emplace() method to insert them without having to
move them.
This avoids the situation of forgetting to add an ID to the cache entry.
This fixes a few places where we can avoid double lookups:

- In RunOnMainThreadFdCallback, we already have a valid iterator, so no
  need to look up the same key again in RemoveKey (this is the only usage
  of RemoveKey, so we can remove it.) (Additionally, we probably want to
  remove the cache entry before throwing a NativeScript exception.)

- In PostFrameCallback and RemoveFrameCallback, we should not do
  contains() immediately followed by find().
@cla-bot cla-bot bot added the cla: yes label Apr 7, 2023
We don't do anything with the return value from these callbacks, so it's
OK to ignore them and not convert them to a local handle.
@ptomato ptomato force-pushed the postFrameCallback branch from a038492 to bca974b Compare May 8, 2023 21:52
@triniwiz triniwiz merged commit ff1b979 into NativeScript:main May 16, 2023
@ptomato ptomato deleted the postFrameCallback branch May 16, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants