Marking mode none #829

PanayotCankov · 2017-08-18T14:29:53Z

Allows the MarkReachableObjects to be suppressed.
Setting:

{
  "android": {
    "markingMode": "none"
  }
}

Will completely disable the JavaScript heap traversal in MarkReachableObjects. This saves about 500ms. block on the main thread (we've seen it go up to 1350ms during navigation)

The memory management model changes drastically:

When a Java instance marshalled to a JavaScript type, the Java and JavaScript instances are bound,
- Add finaliser to the JavaScript instance
- Add a strong reference to the Java instance (this will not let it be garbage collected)
When the JavaScript instance's finaliser is called (the JavaScript instance is not reachable)
- Make the reference to the Java instance weak (this will let it be garbage collected by Java GC)
- Check the weak reference to the Java instance, if the Java instance is still alive, don't let the JavaScript instance be collected

This approach can lead to instances being prematurely collected, but doesn't involve long blocks on the main thread.

We want to run excessive testing on our apps so we can get a better understanding of the implications following from suppression MarkReachableObjects.

Plamen5kov · 2017-08-24T08:29:27Z

runtime/src/main/java/com/tns/Runtime.java


+    public int getMarkingMode() {
+        String mode = staticConfiguration == null || staticConfiguration.appConfig == null ? "full" : staticConfiguration.appConfig.getMarkingMode();
+        if ("none".equals(mode)) {


make enumeration with values of marking mode

Plamen5kov · 2017-08-24T08:33:41Z

runtime/src/main/jni/ObjectManager.cpp

+        m_numberOfGC(0),
+        m_currentObjectId(0),
+        m_cache(NewWeakGlobalRefCallback, DeleteWeakGlobalRefCallback, 1000, this),
+        m_markingMode(JavaScriptMarkingMode::Full) {


get marking mode from java static configuration, don't set it as default here, because it's easier to change defaults only in one place

Plamen5kov · 2017-08-24T08:35:04Z

runtime/src/main/java/com/tns/Runtime.java

        return dynamicConfig;
    }

+    public int getMarkingMode() {


add @RuntimeCallable on method

Plamen5kov · 2017-08-24T08:41:43Z

runtime/src/main/jni/ObjectManager.cpp

+    jint markingMode = m_env.CallIntMethod(m_javaRuntimeObject, getMarkingModeMethodID);
+    switch(markingMode) {
+        case 1:
+            m_markingMode = JavaScriptMarkingMode::None;


note: this means every new thread will have this value reinitialized, so potentially values may differ between threads.

Plamen5kov · 2017-08-24T08:51:17Z

runtime/src/main/jni/ObjectManager.cpp

+                jsInfo = prototypeObject->GetInternalField(jsInfoIdx);
+            }
+        }
+    }


extract repeating code:

const int jsInfoIdx = static_cast<int>(MetadataNodeKeys::JsInfo); auto jsInfo = object->GetInternalField(jsInfoIdx); if (jsInfo->IsUndefined()) { //Typescript object layout has an object instance as child of the actual registered instance. checking for that auto prototypeObject = object->GetPrototype().As<Object>(); if (!prototypeObject.IsEmpty() && prototypeObject->IsObject()) { DEBUG_WRITE("GetJSInstanceInfo: need to check prototype :%d", prototypeObject->GetIdentityHash()); if (IsJsRuntimeObject(prototypeObject)) { jsInfo = prototypeObject->GetInternalField(jsInfoIdx); } } }

used in ObjectManager::GetJSInstanceInfo

Plamen5kov · 2017-08-24T08:58:27Z

runtime/src/main/jni/ObjectManager.cpp

+        }
+    }
+
+    if (jsInfo.IsEmpty() || !jsInfo->IsExternal()) {


!jsInfo->IsExternal() check is redundant

It can be "undefined".

Or at least it used to be before we augmented the previous PR.

ok in that case leave IsEmpty and IsUndefined, but in both cases they implicitly imply !IsExternal

IDK, the whole thing got reorganised when I extracted a "GetJSInstanceInfoFromRuntimeObject" from the "GetJSInstanceInfo" and this one. Take a look now.

Plamen5kov · 2017-08-24T08:59:31Z

runtime/src/main/jni/ObjectManager.cpp

+    }
+
+    if (jsInfo.IsEmpty() || !jsInfo->IsExternal()) {
+        // The JavaScript instance has been forcefully disconnected from the Java instance.


please provide a better comment as this one implies we are setting jsInfo to undefined or empty

We may add explicit "release()" method in future (although highly unlikely). But it also does some sanity check like for the preconditions that we have to check and you mention two lines below.

Plamen5kov · 2017-08-24T09:55:55Z

runtime/src/main/jni/ObjectManager.cpp

+        return;
+    }
+
+    auto external = jsInfo.As<External>();


check !jsInfo.IsEmpty() || jsInfo->IsExternal()) before cast

Actually the method 2 lines above does that check.

before the cast is made there needs to be explicit check for IsExternal

Plamen5kov · 2017-08-24T10:00:54Z

runtime/src/main/jni/ObjectManager.cpp

+    thisPtr->JSObjectFinalizer(isolate, callbackState);
+}
+
+void ObjectManager::JSObjectFinalizer(Isolate* isolate, ObjectWeakCallbackState* callbackState) {


add Handle scope in the beginning of the method to take care of the local handles:

auto isolate = m_isolate; HandleScope handleScope(isolate);

Plamen5kov · 2017-08-24T10:03:16Z

runtime/src/main/jni/ObjectManager.cpp

+    jboolean isJavaInstanceAlive = m_env.CallBooleanMethod(m_javaRuntimeObject, MAKE_INSTANCE_WEAK_AND_CHECK_IF_ALIVE_METHOD_ID, javaObjectID);
+    if (isJavaInstanceAlive) {
+        // If the Java instance is alive, keep the JavaScript instance alive.
+        // TODO: Check should we really register the finalizer again?


if this TODO: is obsolete, remove it, if it's not let's check it out before it gets merged into master.

It was added in the previous implementation in JSObjectWeakCallback so I guess it should be there.

…to switch the memory management to one, that will not traverse the JS heap graph

Plamen5kov

Check out the comments

PanayotCankov · 2017-08-24T12:58:08Z

run ci

ns-bot · 2017-08-24T13:12:47Z

💚

Plamen5kov · 2017-08-25T05:36:20Z

runtime/src/main/jni/ObjectManager.cpp

-
-    if (jsInfo.IsEmpty() || !jsInfo->IsExternal()) {
-        // The JavaScript instance has been forcefully disconnected from the Java instance.
+    if (!jsInstanceInfo) {


in case jsInstanceInfo is nullptr this check wont pass
please change check to: jsInstanceInfo != nullptr

Plamen5kov · 2017-08-25T05:46:10Z

runtime/src/main/java/com/tns/AppConfig.java

-            Profiling,
-            MarkingMode
-        };
+        MarkingMode("markingMode", com.tns.MarkingMode.values()[0]);


com.tns.MarkingMode.full as default value instead of com.tns.MarkingMode.values()[0]

Plamen5kov · 2017-08-25T06:48:38Z

runtime/src/main/java/com/tns/AppConfig.java

+        return (String)values[KnownKeys.Profiling.ordinal()];
    }
-    public String getMarkingMode() { return (String)values[KnownKeys.MarkingMode.getIndex()]; }
+    public MarkingMode getMarkingMode() { return (MarkingMode)values[KnownKeys.MarkingMode.ordinal()]; }


no need for cast (MarkingMode)

The values is of type Object[], indexing it returns Object.

Plamen5kov · 2017-08-25T06:53:52Z

runtime/src/main/java/com/tns/Runtime.java

+        if (staticConfiguration != null && staticConfiguration.appConfig != null) {
+            return staticConfiguration.appConfig.getMarkingMode().ordinal();
+        } else {
+            return MarkingMode.values()[0].ordinal();


use KnownKeys.MarkingMode.getDefaultValue() here, or if you don't want to expose KnownKeys just expose API from app config that gets the default and current if any.

I exposed KnownKeys, I think it is fine.

Plamen5kov · 2017-08-25T07:00:34Z

runtime/src/main/java/com/tns/AppConfig.java

+                        try {
+                            String value = androidObject.getString(KnownKeys.MarkingMode.getName());
+                            values[KnownKeys.MarkingMode.ordinal()] = MarkingMode.valueOf(value);
+                        } catch(Throwable e) {


catch Exception instead of throwable here if you don't want to have a specific warning for each exception, otherwise catch the specific exceptions thrown my methods.

don't leave catch block empty, please leave a warning that the default value will be used because we couldn't get the value for some reason

Printed the stack trace and then a message with Log.v("JS", ... that the default will be used.

Plamen5kov

Check out comments.
ordinal() is a great addition.

PanayotCankov · 2017-08-25T10:22:10Z

runtime/src/main/java/com/tns/AppConfig.java

            rootObject = FileSystem.readJSONFile(packageInfo);
            if (rootObject != null) {
-                if (rootObject.has(KnownKeys.Profiling.getName())) {
+                if (rootObject.has(KnownKeys.Profiling.name())) {


this should be getName()

Plamen5kov

After green build

PanayotCankov · 2017-08-25T11:33:54Z

run ci

ns-bot · 2017-08-25T13:20:27Z

💚

PanayotCankov force-pushed the marking-mode-none branch from 6a17540 to 8447e71 Compare August 21, 2017 08:29

NativeScript deleted a comment from ns-bot Aug 21, 2017

NativeScript deleted a comment from dtopuzov Aug 21, 2017

NativeScript deleted a comment from ns-bot Aug 21, 2017

Plamen5kov suggested changes Aug 24, 2017

View reviewed changes

Plamen5kov reviewed Aug 24, 2017

View reviewed changes

NativeScript deleted a comment from ns-bot Aug 24, 2017

NativeScript deleted a comment from dtopuzov Aug 24, 2017

NativeScript deleted a comment from ns-bot Aug 24, 2017

NativeScript deleted a comment from dtopuzov Aug 24, 2017

NativeScript deleted a comment from ns-bot Aug 24, 2017

NativeScript deleted a comment from dtopuzov Aug 24, 2017

NativeScript deleted a comment from ns-bot Aug 24, 2017

NativeScript deleted a comment from dtopuzov Aug 24, 2017

NativeScript deleted a comment from ns-bot Aug 24, 2017

Add "markingMode": "none" to the android key in the app/package.json …

7092883

…to switch the memory management to one, that will not traverse the JS heap graph

Plamen5kov suggested changes Aug 24, 2017

View reviewed changes

PanayotCankov force-pushed the marking-mode-none branch 2 times, most recently from 49d0556 to 0e6e7e3 Compare August 24, 2017 12:10

NativeScript deleted a comment from ns-bot Aug 24, 2017

Apply some refactoring after PR reviews

052b28e

PanayotCankov force-pushed the marking-mode-none branch from 0e6e7e3 to 052b28e Compare August 24, 2017 12:53

NativeScript deleted a comment from ns-bot Aug 24, 2017

Plamen5kov reviewed Aug 25, 2017

View reviewed changes

Plamen5kov suggested changes Aug 25, 2017

View reviewed changes

PanayotCankov force-pushed the marking-mode-none branch from f1d95b9 to 9f8bfcd Compare August 25, 2017 09:15

PanayotCankov commented Aug 25, 2017

View reviewed changes

PanayotCankov force-pushed the marking-mode-none branch from 9f8bfcd to 49c7d77 Compare August 25, 2017 10:28

Addressed some comments in PR review

1708cba

PanayotCankov force-pushed the marking-mode-none branch from 49c7d77 to 1708cba Compare August 25, 2017 10:33

Plamen5kov approved these changes Aug 25, 2017

View reviewed changes

NativeScript deleted a comment from ns-bot Aug 25, 2017

PanayotCankov merged commit 93760a0 into master Aug 25, 2017

PanayotCankov deleted the marking-mode-none branch August 25, 2017 14:13

NickIliev mentioned this pull request Mar 1, 2018

Nativescript + Angular , "random" UI FREEZE and ANR issues! NativeScript/NativeScript#4204

Closed

NickIliev mentioned this pull request Oct 10, 2018

Freeze after: JS garbage collection encountered invalid thread NativeScript/NativeScript#6307

Closed

Uh oh!

Marking mode none #829

Marking mode none #829

Uh oh!

Conversation

PanayotCankov commented Aug 18, 2017

Uh oh!

Plamen5kov Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov left a comment

Choose a reason for hiding this comment

Uh oh!

PanayotCankov commented Aug 24, 2017

Uh oh!

ns-bot commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Plamen5kov Aug 24, 2017 •

edited

Loading

Plamen5kov Aug 24, 2017 •

edited

Loading

Plamen5kov Aug 24, 2017 •

edited

Loading

Plamen5kov Aug 25, 2017 •

edited

Loading

Plamen5kov Aug 25, 2017 •

edited

Loading