zhztheplayer commented on a change in pull request #11067: URL: https://github.com/apache/arrow/pull/11067#discussion_r703229917
########## File path: java/ffi/README.md ########## @@ -0,0 +1,48 @@ +<!--- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +# Java FFI (C Data Interface) + +## Setup Build Environment + +install: + - Java 8 or later + - Maven 3.3 or later + - A C++11-enabled compiler + - CMake 3.11 or later + - Make or ninja build utilities + +## Building JNI wrapper shared library + +``` +mkdir -p ./target/build/ +pushd ./target/build/ +cmake ../.. +make +popd +``` + +To use ninja, pass `-GNinja` when calling cmake and then use the `ninja` command instead of `make`. + +## Building and running tests + +``` +cd java +mvn -Parrow-ffi install Review comment: So clean build is not allowed here right? As we already put all cpp build files to `target/`. ########## File path: java/ffi/src/main/cpp/jni_wrapper.cc ########## @@ -0,0 +1,240 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <jni.h> + +#include <cassert> +#include <memory> +#include <string> + +#include "abi.h" +#include "org_apache_arrow_ffi_jni_JniWrapper.h" + +namespace +{ + + jclass CreateGlobalClassReference(JNIEnv *env, const char *class_name) + { + jclass local_class = env->FindClass(class_name); + jclass global_class = (jclass)env->NewGlobalRef(local_class); + env->DeleteLocalRef(local_class); + return global_class; + } + + jclass illegal_access_exception_class; + jclass illegal_argument_exception_class; + jclass runtime_exception_class; + jclass private_data_class; + + jmethodID private_data_close_method; + + jint JNI_VERSION = JNI_VERSION_1_6; + + class JniPendingException : public std::runtime_error Review comment: I was getting error `arrow/java/ffi/src/main/cpp/jni_wrapper.cc:49:78: error: expected class-name before ‘(’ token`until explicitly including stdexcept in this file. (gcc 10.1.1) ########## File path: java/ffi/src/main/java/org/apache/arrow/ffi/FFIReferenceManager.java ########## @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.arrow.ffi; + +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.arrow.memory.ArrowBuf; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.memory.OwnershipTransferResult; +import org.apache.arrow.memory.ReferenceManager; +import org.apache.arrow.util.Preconditions; + +/** + * A ReferenceManager implementation that holds a {@link org.apache.arrow.ffi.BaseStruct}. + * <p> + * A reference count is maintained and once it reaches zero the struct + * is released (as per the C data interface specification) and closed. + */ +final class FFIReferenceManager implements ReferenceManager { + private final AtomicInteger bufRefCnt = new AtomicInteger(0); + + private final BaseStruct struct; + + FFIReferenceManager(BaseStruct struct) { + this.struct = struct; + } + + @Override + public int getRefCount() { + return bufRefCnt.get(); + } + + @Override + public boolean release() { + return release(1); + } + + @Override + public boolean release(int decrement) { + Preconditions.checkState(decrement >= 1, "ref count decrement should be greater than or equal to 1"); + // decrement the ref count + final int refCnt; + synchronized (this) { + refCnt = bufRefCnt.addAndGet(-decrement); + if (refCnt == 0) { + // refcount of this reference manager has dropped to 0 + // release the underlying memory + struct.release(); + struct.close(); + } + } + // the new ref count should be >= 0 + Preconditions.checkState(refCnt >= 0, "RefCnt has gone negative"); + return refCnt == 0; + } + + @Override + public void retain() { + retain(1); + } + + @Override + public void retain(int increment) { + Preconditions.checkArgument(increment > 0, "retain(%s) argument is not positive", increment); + bufRefCnt.addAndGet(increment); + } + + @Override + public ArrowBuf retain(ArrowBuf srcBuffer, BufferAllocator targetAllocator) { + retain(); + return srcBuffer; Review comment: Did we make a trade-off between returning the source buffer or creating a new buffer here? I am not sure which way is better but it seems that in this way the source buffer will be shared to the target vector along with its internal states (read / write indexes) during buffer loading, although an allocator was assigned to that vector. Also, here we may fail to register any buffer to the `targetAllocator`. But maybe it doesn't really hurt and be too tricky to improve. ########## File path: java/ffi/README.md ########## @@ -0,0 +1,48 @@ +<!--- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +# Java FFI (C Data Interface) + +## Setup Build Environment + +install: + - Java 8 or later + - Maven 3.3 or later + - A C++11-enabled compiler + - CMake 3.11 or later + - Make or ninja build utilities + +## Building JNI wrapper shared library + +``` +mkdir -p ./target/build/ +pushd ./target/build/ +cmake ../.. +make +popd +``` Review comment: Similar to previous comment... I am worried about if we introduce more complexity for keeping the independency of ffi cpp code. Would it be better to somehow reorganize related source files into cpp folder (maybe under `jni/`)? We can still include ffi's own lib into its jar file during `mvn install`. ########## File path: java/ffi/src/main/cpp/abi.h ########## @@ -0,0 +1,103 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include <stdint.h> + +#ifdef __cplusplus +extern "C" { +#endif + +#define ARROW_FLAG_DICTIONARY_ORDERED 1 +#define ARROW_FLAG_NULLABLE 2 +#define ARROW_FLAG_MAP_KEYS_SORTED 4 + +struct ArrowSchema { Review comment: Do we have to duplicate this file? ########## File path: java/ffi/src/main/cpp/jni_wrapper.cc ########## @@ -0,0 +1,240 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <jni.h> + +#include <cassert> +#include <memory> +#include <string> + +#include "abi.h" +#include "org_apache_arrow_ffi_jni_JniWrapper.h" + +namespace +{ + + jclass CreateGlobalClassReference(JNIEnv *env, const char *class_name) + { + jclass local_class = env->FindClass(class_name); + jclass global_class = (jclass)env->NewGlobalRef(local_class); + env->DeleteLocalRef(local_class); + return global_class; + } + + jclass illegal_access_exception_class; + jclass illegal_argument_exception_class; + jclass runtime_exception_class; + jclass private_data_class; + + jmethodID private_data_close_method; + + jint JNI_VERSION = JNI_VERSION_1_6; + + class JniPendingException : public std::runtime_error + { + public: + explicit JniPendingException(const std::string &arg) : std::runtime_error(arg) {} + }; + + void ThrowPendingException(const std::string &message) + { + throw JniPendingException(message); + } + + void JniThrow(std::string message) { ThrowPendingException(message); } + + jmethodID GetMethodID(JNIEnv* env, jclass this_class, const char* name, + const char* sig) { + jmethodID ret = env->GetMethodID(this_class, name, sig); + if (ret == nullptr) { + std::string error_message = "Unable to find method " + std::string(name) + + " within signature " + std::string(sig); + ThrowPendingException(error_message); + } + return ret; + } + + class InnerPrivateData + { + public: + InnerPrivateData(JavaVM* vm, jobject private_data) + : vm_(vm), j_private_data_(private_data) {} + + JavaVM* vm_; + jobject j_private_data_; + }; + + template<typename T> + void release_exported(T* base) { + // This should not be called on already released structure + assert(base->release != nullptr); + + // Release children + for (int64_t i = 0; i < base->n_children; ++i) { + T* child = base->children[i]; + if (child->release != nullptr) { + child->release(child); + assert(child->release == nullptr); + } + } + + // Release dictionary + T* dict = base->dictionary; + if (dict != nullptr && dict->release != nullptr) { + dict->release(dict); + assert(dict->release == nullptr); + } + + // Release all data directly owned by the struct + InnerPrivateData* private_data = reinterpret_cast<InnerPrivateData*>(base->private_data); + JNIEnv* env; + if (private_data->vm_->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION) != JNI_OK) { + ThrowPendingException("JNIEnv was not attached to current thread"); + } Review comment: Would it be better to use `vm_->AttachCurrentThread` instead? As the exported data might be imported and released from other threads that are not attached to JVM. We may allow this `GetEnv` routine in other modules but that was because in those modules we don't have to deal with native-created threads. https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/invocation.html#AttachCurrentThread -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
