Add async exec call #28
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ospfranco
added
enhancement
|
@ospfranco, do you belive that I was working on it for the last few days and was almost giving up? So many crashes trying to use jsCallInvoker. I will try it RIGHT NOW. :). |
|
@ospfranco I've tryed on Android 11 (Emulator and real device), with and without Hermes. This is the JavaScript call: Error Without Hermes: Error with Hermes: |
|
I've had to update the sample app to use RN 0.67, on the previous version 0.63 it was crashing, you might try that. Also, isn't your call missing the first param (db name)? |
I will update to 0.67 here, no problem. I will redoo all the tests with 0.67 version and let you know. Thank you very mutch for your feedback. |
|
@ospfranco Upgraded my project to RN 0.67.3, and now it works, but I need to do a tweak on my code to avoid new calls to be dispatched to quick sql before the async method promise was called. I have some good scenarios where I can test this new approach and I will write a sequel_asyncExecSQLBatch right now. |
|
I tried to implement the batch update method but somehow it fails when resolving the promise. My code. It fails with: "facebook::jsi::Function::call" - I belive that is the promise resolver callback. I've read some threads this -> Yesterday I was trying to use jsInvoker.invokeAsync() but it crashes. Do you have heard of it right? |
|
This repository appears to do exactly what we are looking for: https://github.com/barthap/discovering-turbomodules At this point: What do you think? |
|
Yeah, I noticed when doing a call immediately before the async method it would crash (only on android), and I've been trying to convert the call to a callback, but that also crashes somehow. Yeah, I remember reading about the callInvoker but I never got it to work, if I remember correctly it also required Turbo Modules, which I never finished figuring it out. In any case, I don't think I'll manage to get any further this week, if you can get it to work then feel free to open a PR to this PR and I can review it later. |
Sure. I will continue to work on it this week! Thank you for your code review and this PR. |
sergeymild
reviewed
Mar 10, 2022
|
|
||
| if (result.type == SequelResultError) | ||
| { | ||
| resolver->asFunction(rt).call(rt, createError(rt, result.message.c_str())); |
There was a problem hiding this comment.
it will not work, you should call js functions in js therad, so you need jsCallbackInvoker from bridge and exec method invokeAsync.
Like here: https://github.com/sergeymild/react-native-jsi-bridge/blob/3ac5415b785fc6f8be51acf630d9958ffa7e6c33/ios/_JsiBridge.mm#L100
There was a problem hiding this comment.
Yeah, I've been trying to get fbjni and the callInvoker work in #29 but so far I cannot get android to compile with the jni, looking at your repo, I've tried to replicate the build.gradle and the CMakeLists but it is way too complex for me
There was a problem hiding this comment.
i have watched your video on youtube, i know why on android you have a crashes, it because you looked at my code and at logcat errors not very well) to pass java object like jsInvoker to jni you need to rewrite jni communication between java and c++. https://github.com/sergeymild/react-native-jsi-sqlite - here i created temporally library there i did everything like it must be on android. And it works as expected. Also you don't need to rewrite sqlite.cpp. First of all you need create Hybrid c++ class which will be like a mirror of your java class for example SequelModule.java. next do everything like i did.
There was a problem hiding this comment.
currently it's only implemented on android to show you that you did wrong
There was a problem hiding this comment.
currently it's only implemented on android to show you that you did wrong
Using your examples, Its working now with this changes:
public class NativeProxy {
private native void installNativeJsi(long jsContextNativePointer, CallInvokerHolderImpl jsCallInvokerHolder, String docDir);
public static final NativeProxy instance = new NativeProxy();
public void installJsi(ReactContext context) {
Log.d("rn-native-quick-sqlite", "Installing native...");
CallInvokerHolderImpl holder = (CallInvokerHolderImpl)context.getCatalystInstance().getJSCallInvokerHolder();
long contextPointer = context.getJavaScriptContextHolder().get();
final String path = context.getFilesDir().getAbsolutePath();
installNativeJsi(contextPointer, holder, path);
}
}
#include <jni.h>
#include <fbjni/fbjni.h>
#include <jsi/jsi.h>
#include <ReactCommon/CallInvokerHolder.h>
#include "react-native-quick-sqlite.h"
#include "logs.h"
#include <typeinfo>
struct NativeProxy : jni::JavaClass<NativeProxy> {
static constexpr auto kJavaDescriptor = "Lcom/reactnativesequel/NativeProxy;";
static JavaVM* vm;
static void registerNatives(JavaVM* v) {
//NativeProxy::vm = v;
// register native methods for Java
javaClassStatic()->registerNatives({
//initialization for JSI
makeNativeMethod("installNativeJsi", NativeProxy::installNativeJsi)
});
}
private:
static void installNativeJsi(jni::alias_ref<jni::JObject> thiz,
jlong jsiRuntimePtr,
jni::alias_ref<react::CallInvokerHolder::javaobject> jsCallInvokerHolder,
jni::alias_ref<jni::JString> docPath) {
auto jsiRuntime = reinterpret_cast<jsi::Runtime*>(jsiRuntimePtr);
auto jsCallInvoker = jsCallInvokerHolder->cthis()->getCallInvoker();
std::string docPathString = docPath->toStdString();
installSequel(*jsiRuntime, jsCallInvoker, docPathString.c_str());
}
};
JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void*) {
return jni::initialize(vm, [vm] {
NativeProxy::registerNatives(vm);
});
}
Thank you.
There was a problem hiding this comment.
@EduFrazao your code looks a lot simpler, would you mind opening a PR so I can review it?
Sure. I will do it right now. In this moment I'm testing the loadSQLFileAsync method. I will submit it in few minutes and let you know if its working. I will test with several files, some of them with more than 120k rows.
Today I run another tests, dispatching more than 500 async calls with a random thread wait time to simulate different workloads. All of them work as expected, and all callbacks works as expected.
There was a problem hiding this comment.
well... I got android to compile and I got the call to the callInvoker working, but somehow android it is still crashing. Just open your PR asap, I would like to understand how did you got it working
There was a problem hiding this comment.
@EduFrazao your code looks a lot simpler, would you mind opening a PR so I can review it?
Finished my testes here. Async loading of SQLFiles is working, and the applicatoins runs so smooth :).
More than 250k rows background processed, and the UI keeps responsive :).
I will open a PR.
There was a problem hiding this comment.
I tried to replicate your changes with a struct but somehow android is crashing for me when I try to register theJSI bindings
There was a problem hiding this comment.
I tried to replicate your changes with a struct but somehow android is crashing for me when I try to register theJSI bindings
Can I help some how? It fails registering the methods on the sqlite module? I've made tests with the SQLFileLoader and a test method that does nothing, except create some random numbers, put the working thread to sleep (to simulate some workload time).
|
closing this in favor of #29, there the jsCallInvoker is correctly linked and things are working (on android) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Managed to implement spawning a C++ thread and returning the result of the SQL execution
@EduFrazao would you mind giving it a test? Try to run your queries with the
asyncExecuteSqlmethodTODOS: