awtterpip / bevy_oxr

Apache License 2.0
253 stars 39 forks source link

[Android] Creating temporary JNIEnv. This is a heavy operation and should be infrequent. To optimize, use JNI AttachCurrentThread on calling thread #96

Closed LogicFan closed 4 months ago

LogicFan commented 7 months ago

Looking at some other project, this issue seems to be oculus exclusive. https://github.com/ejeinc/Meganekko/blob/master/ovr_sdk_mobile/LibOVRKernel/Src/Android/JniUtils.h

This issue exists because when oculus cannot find jvm attached, it will create a temporary one and delete afterwards. Since bevy is multi-threaded, and all those new thread created by bevy does not have jvm attached, when the bevy system which invoke the oculus function runs on a thread other than main, we will observe this warning.

This can be resolved by having

    ComputeTaskPool::get_or_init(|| {
        TaskPoolBuilder::default()
            .num_threads(10)
            .on_thread_spawn(|| {
                let ctx = ndk_context::android_context();
                let vm = unsafe { jni::JavaVM::from_raw(ctx.vm().cast()) }.unwrap();
                vm.attach_current_thread_permanently();
            })
            .thread_name("IO Task Pool".to_string())
            .build()
    });

before the bevy app code, which will override task pool creation later at bevy task pool plugin. However this method is hacky and an proper method should be considered.

ForTehLose commented 7 months ago

Why is this considered hacky? Seems to me you are attaching every thread to the jvm.

LogicFan commented 7 months ago

Why is this considered hacky? Seems to me you are attaching every thread to the jvm.

Sorry I did not explain myself clearly. It's not hacky in terms of attaching jvm, It's hacky in terms of how to override the bevy task pool. I currently have a PR to allow us configure this through bevy TaskPoolPlugin here: https://github.com/bevyengine/bevy/pull/13045

Also since this is oculus exclusive, should we hide this behind a feature flag so it won't interfere with other platform?

MalekiRe commented 4 months ago

Fixed