Merge pull request #423 from NativeScript/plamen5kov/object_manager_refactor

Plamen5kov/object manager refactor

Author: Plamen5kov

src/jni/CallbackHandlers.h

@@ -7,7 +7,6 @@
 #include "v8.h"
 #include "v8-debug.h"
 #include "JEnv.h"
-#include "JSInstanceInfo.h"
 #include "ArgsWrapper.h"
 #include "MetadataEntry.h"
 #include "FieldCallbackData.h"

src/jni/DirectBuffer.cpp

@@ -52,7 +52,7 @@ bool DirectBuffer::Write(int value)
 	if (canWrite)
 	{
 		int bigEndianInt = __builtin_bswap32(value);
-		*m_pos++ = bigEndianInt;
+		*(m_pos++) = bigEndianInt;
 	}
 	return canWrite;
 }

src/jni/JSInstanceInfo.h

@@ -82,19 +82,17 @@ JniLocalRef ObjectManager::GetJavaObjectByJsObject(const Local<Object>& object)
 	return JniLocalRef();
 }
 
-JSInstanceInfo* ObjectManager::GetJSInstanceInfo(const Local<Object>& object)
+ObjectManager::JSInstanceInfo* ObjectManager::GetJSInstanceInfo(const Local<Object>& object)
 {
 	DEBUG_WRITE("ObjectManager::GetJSInstanceInfo: called");
 	JSInstanceInfo *jsInstanceInfo = nullptr;
 
 	auto isolate = Isolate::GetCurrent();
 	HandleScope handleScope(isolate);
 
-	int internalFieldCound = NativeScriptExtension::GetInternalFieldCount(object);
-	const int count = static_cast<int>(MetadataNodeKeys::END);
-	const int jsInfoIdx = static_cast<int>(MetadataNodeKeys::JsInfo);
-	if (internalFieldCound == count)
+	if (IsJsRuntimeObject(object))
 	{
+		const int jsInfoIdx = static_cast<int>(MetadataNodeKeys::JsInfo);
 		auto jsInfo = object->GetInternalField(jsInfoIdx);
 		if (jsInfo->IsUndefined())
 		{
@@ -104,8 +102,7 @@ JSInstanceInfo* ObjectManager::GetJSInstanceInfo(const Local<Object>& object)
 			if (!prototypeObject.IsEmpty() && prototypeObject->IsObject())
 			{
 				DEBUG_WRITE("GetJSInstanceInfo: need to check prototype :%d", prototypeObject->GetIdentityHash());
-				internalFieldCound = NativeScriptExtension::GetInternalFieldCount(prototypeObject);
-				if (internalFieldCound == count)
+				if (IsJsRuntimeObject(prototypeObject))
 				{
 					jsInfo = prototypeObject->GetInternalField(jsInfoIdx);
 				}
@@ -122,6 +119,12 @@ JSInstanceInfo* ObjectManager::GetJSInstanceInfo(const Local<Object>& object)
 	return jsInstanceInfo;
 }
 
+bool ObjectManager::IsJsRuntimeObject(const v8::Local<v8::Object>& object) {
+	int internalFieldCount = NativeScriptExtension::GetInternalFieldCount(object);
+	const int count = static_cast<int>(MetadataNodeKeys::END);
+	return internalFieldCount == count;
+}
+
 jweak ObjectManager::GetJavaObjectByID(uint32_t javaObjectID)
 {
 	jweak obj = m_cache(javaObjectID);
@@ -145,7 +148,7 @@ jclass ObjectManager::GetJavaClass(const Local<Object>& instance)
 	DEBUG_WRITE("GetClass called");
 
 	JSInstanceInfo *jsInfo = GetJSInstanceInfo(instance);
-	jclass clazz = jsInfo->clazz;
+	jclass clazz = jsInfo->ObjectClazz;
 
 	return clazz;
 }
@@ -155,7 +158,7 @@ void ObjectManager::SetJavaClass(const Local<Object>& instance, jclass clazz)
 	DEBUG_WRITE("SetClass called");
 
 	JSInstanceInfo *jsInfo = GetJSInstanceInfo(instance);
-	jsInfo->clazz = clazz;
+	jsInfo->ObjectClazz = clazz;
 }
 
 int ObjectManager::GetOrCreateObjectId(jobject object)
@@ -213,11 +216,13 @@ Local<Object> ObjectManager::CreateJSWrapperHelper(jint javaObjectID, const stri
 	return jsWrapper;
 }
 
+
+/* *
+ * Link the JavaScript object and it's java counterpart with an ID
+ */
 void ObjectManager::Link(const Local<Object>& object, uint32_t javaObjectID, jclass clazz)
 {
-	int internalFieldCound = NativeScriptExtension::GetInternalFieldCount(object);
-	const int count = static_cast<int>(MetadataNodeKeys::END);
-	if (internalFieldCound != count)
+	if (!IsJsRuntimeObject(object))
 	{
 		string errMsg("Trying to link invalid 'this' to a Java object");
 		throw NativeScriptException(errMsg);
@@ -227,19 +232,19 @@ void ObjectManager::Link(const Local<Object>& object, uint32_t javaObjectID, jcl
 
 	DEBUG_WRITE("Linking js object: %d and java instance id: %d", object->GetIdentityHash(), javaObjectID);
 
-	auto jsInstanceInfo = new JSInstanceInfo();
-	jsInstanceInfo->JavaObjectID = javaObjectID;
-	jsInstanceInfo->clazz = clazz;
+	auto jsInstanceInfo = new JSInstanceInfo(false/*isJavaObjWeak*/, javaObjectID, clazz);
 
 	auto objectHandle = new Persistent<Object>(isolate, object);
 	auto state = new ObjectWeakCallbackState(this, jsInstanceInfo, objectHandle);
+
+	// subscribe for JS GC event
 	objectHandle->SetWeak(state, JSObjectWeakCallbackStatic);
 
 	auto jsInfoIdx = static_cast<int>(MetadataNodeKeys::JsInfo);
-	bool alreadyLinked = !object->GetInternalField(jsInfoIdx)->IsUndefined();
-	//TODO: fail if alreadyLinked is true?
 
 	auto jsInfo = External::New(isolate, jsInstanceInfo);
+
+	//link
 	object->SetInternalField(jsInfoIdx, jsInfo);
 
 	idToObject.insert(make_pair(javaObjectID, objectHandle));
@@ -290,6 +295,14 @@ void ObjectManager::JSObjectWeakCallbackStatic(const WeakCallbackData<Object, Ob
 	thisPtr->JSObjectWeakCallback(isolate, callbackState);
 }
 
+/*
+ * When JS GC happens change state of the java counterpart to mirror state of JS object and REVIVE the JS object unconditionally
+ * "Regular" js objects are pushed into the "regular objects" array
+ * "Callback" js objects (ones that have implementation object) are pushed into two "special objects" array:
+ * 		-ones called for the first time which are originally strong in java
+ * 		-ones called for the second or next time which are already weak in java too
+ *	These objects are categorized by "regular" and "callback" and saved in different arrays for performance optimizations during GC
+ * */
 void ObjectManager::JSObjectWeakCallback(Isolate *isolate, ObjectWeakCallbackState *callbackState)
 {
 	DEBUG_WRITE("JSObjectWeakCallback called");
@@ -305,7 +318,6 @@ void ObjectManager::JSObjectWeakCallback(Isolate *isolate, ObjectWeakCallbackSta
 		m_visitedPOs.insert(po);
 
 		auto obj = Local<Object>::New(isolate, *po);
-
 		JSInstanceInfo *jsInstanceInfo = GetJSInstanceInfo(obj);
 		int javaObjectID = jsInstanceInfo->JavaObjectID;
 
@@ -370,6 +382,9 @@ void ObjectManager::ReleaseJSInstance(Persistent<Object> *po, JSInstanceInfo *js
 	DEBUG_WRITE("ReleaseJSObject instance disposed. id:%d", javaObjectID);
 }
 
+/*
+ * The "regular" JS objects added on ObjectManager::JSObjectWeakCallback are dealt with(released) here.
+ * */
 void ObjectManager::ReleaseRegularObjects()
 {
 	Isolate *isolate = Isolate::GetCurrent();
@@ -399,6 +414,7 @@ void ObjectManager::ReleaseRegularObjects()
 		{
 			int objGcNum = gcNum->Int32Value();
 
+			// done so we can release only java objects from this GC stack and pass all objects that will be released in parent GC stacks
 			isReachableFromImplementationObject = objGcNum >= numberOfGC;
 		}
 
@@ -427,6 +443,10 @@ bool ObjectManager::HasImplObject(Isolate *isolate, const Local<Object>& obj)
 	return hasImplObj;
 }
 
+/*
+ * When "MarkReachableObjects" is called V8 has marked all JS objects that can be released.
+ * This method builds on top of V8s marking phase, because we need to consider the JAVA counterpart objects (is it "regular" or "callback"), when marking JS ones.
+ * */
 void ObjectManager::MarkReachableObjects(Isolate *isolate, const Local<Object>& obj)
 {
 	stack<Local<Value> > s;
@@ -461,6 +481,9 @@ void ObjectManager::MarkReachableObjects(Isolate *isolate, const Local<Object>&
 			auto hasImplObject = HasImplObject(isolate, o);
 			if (hasImplObject)
 			{
+				// this is a special case when one "callback1" object (one we are currently traversing)
+				// can reach another "callback2" object (jsInfo->JavaObjectID)
+				// here we are leaving "callback2" object to remain strong in java
 				m_implObjStrong[jsInfo->JavaObjectID] = nullptr;
 			}
 			o->SetHiddenValue(propName, curGCNumValue);
@@ -615,10 +638,14 @@ void ObjectManager::OnGcStarted(GCType type, GCCallbackFlags flags)
 	m_markedForGC.push(gcInfo);
 }
 
+/*
+ * When GC is called we need to evaluate the situation and decide what js objects to release
+ * */
 void ObjectManager::OnGcFinished(GCType type, GCCallbackFlags flags)
 {
 	assert(!m_markedForGC.empty());
 
+	//deal with all "callback" objects
 	auto isolate = Isolate::GetCurrent();
 	for (auto weakObj : m_implObjWeak)
 	{
@@ -635,6 +662,7 @@ void ObjectManager::OnGcFinished(GCType type, GCCallbackFlags flags)
 		}
 	}
 
+	//deal with regular objects
 	ReleaseRegularObjects();
 
 	m_markedForGC.pop();
@@ -656,6 +684,10 @@ void ObjectManager::OnGcFinished(GCType type, GCCallbackFlags flags)
 	}
 }
 
+/*
+ * We have all the JS "regular" objects that JS has made weak and ready to by GC'd,
+ * so we tell java to take the JAVA objects out of strong reference so they can be collected by JAVA GC
+ * */
 void ObjectManager::MakeRegularObjectsWeak(const set<int>& instances, DirectBuffer& inputBuff)
 {
 	jboolean keepAsWeak = JNI_FALSE;
@@ -682,6 +714,11 @@ void ObjectManager::MakeRegularObjectsWeak(const set<int>& instances, DirectBuff
 	inputBuff.Reset();
 }
 
+/*
+ * We have all the JS "callback" objects that JS has made weak and ready to by GC'd,
+ * so we tell java to take the JAVA objects out of strong, BUT KEEP THEM AS WEEK REFERENCES,
+ * so that if java needs to release them, it can, on a later stage.
+ * */
 void ObjectManager::MakeImplObjectsWeak(const map<int, Persistent<Object>*>& instances, DirectBuffer& inputBuff)
 {
 	jboolean keepAsWeak = JNI_TRUE;
@@ -715,6 +752,10 @@ void ObjectManager::MakeImplObjectsWeak(const map<int, Persistent<Object>*>& ins
 	inputBuff.Reset();
 }
 
+/*
+ * Consult with JAVA world to check if a java object is still in kept as a strong or weak reference
+ * If the JAVA objects are released, we can release the their counterpart JS objects
+ * */
 void ObjectManager::CheckWeakObjectsAreAlive(const vector<PersistentObjectIdPair>& instances, DirectBuffer& inputBuff, DirectBuffer& outputBuff)
 {
 	for (const auto& poIdPair : instances)

src/jni/ObjectManager.cpp

@@ -5,7 +5,6 @@
 #include "JEnv.h"
 #include "JniLocalRef.h"
 #include "ArgsWrapper.h"
-#include "JSInstanceInfo.h"
 #include "DirectBuffer.h"
 #include "LRUCache.h"
 #include <map>
@@ -58,6 +57,20 @@ namespace tns
 			};
 
 		private:
+
+			struct JSInstanceInfo
+			{
+				public:
+					JSInstanceInfo(bool isJavaObjectWeak, uint32_t javaObjectID, jclass claz)
+						:IsJavaObjectWeak(isJavaObjectWeak), JavaObjectID(javaObjectID), ObjectClazz(claz)
+					{
+					}
+
+					bool IsJavaObjectWeak;
+					uint32_t JavaObjectID;
+					jclass ObjectClazz;
+			};
+
 			struct ObjectWeakCallbackState
 			{
 					ObjectWeakCallbackState(ObjectManager *_thisPtr, JSInstanceInfo *_jsInfo, v8::Persistent<v8::Object> *_target)
@@ -121,6 +134,8 @@ namespace tns
 					int javaObjectId;
 			};
 
+			bool IsJsRuntimeObject(const v8::Local<v8::Object>& object);
+
 			JSInstanceInfo* GetJSInstanceInfo(const v8::Local<v8::Object>& object);
 
 			void ReleaseJSInstance(v8::Persistent<v8::Object> *po, JSInstanceInfo *jsInstanceInfo);