Object literal issues

[mwaldrich]

Currently, it is not possible to accurately track which values flow into object literals. This is because NodeProf invokes read callbacks for each value in the object literal, and then invokes the literal callback without any information about which order the keys are in.

While it is possible to iterate over the properties of the literal using a for in loop, this does not always give you the order that the keys came in from the literal. For example, when mixing string and number keys, the order becomes inconsistent.

The following is an example analysis and program that demonstrates this issue.

literalAnalysis.js:

// DO NOT INSTRUMENT
(function (sandbox) {
    function MyAnalysis() {
        this.literal = function (iid, val, hasGetterSetter, literalType) {
            console.log("literal " + val);
            if (typeof val === "object") {
                console.log("object property names:");
                for (prop in val) {
                    if (val.hasOwnProperty(prop)) {
                        console.log("- " + prop);
                    }
                }
            }
        };

        this.getField = function (iid, base, offset, val, isComputed, isOpAssign, isMethodCall) {
            console.log("getField " + offset + " " + val);
        };

        this.putField = function (iid, base, offset, val, isComputed, isOpAssign) {
            console.log("putField " + offset + " = " + val);
        };

        this.read = function (iid, name, val, isGlobal, isScriptLocal) {
            console.log("read " + name + " " + val);
        };
        this.write = function (iid, name, val, lhs, isGlobal, isScriptLocal) {
            console.log("write " + name + " = " + val);
        };

        this.declare = function (iid, name, type) {
            console.log("declare " + name);
        };

    }

    sandbox.analysis = new MyAnalysis();
})(J$);

literalExample.js:

var obj = {"b": true, "a": false, "1": 12};
obj.a = 42;

mx jalangi --analysis literalAnalysis.js literalExample.js:

literal function (exports, require, module, __filename, __dirname) { var obj = {"b": true, "a": false, "1": 12};
obj.a = 42;

}
declare exports
declare require
declare module
declare __filename
declare __dirname
declare obj
literal true
literal false
literal 12
literal [object Object]
object property names:
- 1
- b
- a
write obj = [object Object]
read obj [object Object]
literal 42
putField a = 42

The output demonstrates that a for in loop doesn't always order the keys in the same way as the literal. If it did, the output would instead be:

object property names:
- b
- a
- 1

The ordering of the key names should be passed as an argument to a NodeProf callback. Perhaps it should be an argument to the literal callback, keyOrder, but this doesn't make sense for other literal types.

Any ideas?

[Haiyang-Sun]

A hack solution using reflection can be found here: https://github.com/Haiyang-Sun/nodeprof.js/tree/obj-literal.
However, it's not tested for more complicated cases, e.g., for object with getter / setter.

BTW, it's also possible to add hasGetterSetter argument to the literal callback by checking the member nodes.

@eleinadani . If the information can be exposed by ObjectLiteralNode in Graal.js, we can avoid using reflection.

[eleinadani]

@Haiyang-Sun I'm not sure what kind of extra info you'd need from graal.js here -- can you prototype a change to core graal.js or provide an example unit test for the core instr?

[Haiyang-Sun]

@eleinadani
Something like this shall work:

diff --git a/graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/ObjectLiteralNode.java b/graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/ObjectLiteralNode.java
index 6f1fbc334d..65c3739026 100644
--- a/graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/ObjectLiteralNode.java
+++ b/graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/ObjectLiteralNode.java
@@ -40,6 +40,7 @@
  */
 package com.oracle.truffle.js.nodes.access;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.concurrent.locks.Lock;
 
@@ -73,6 +74,7 @@ import com.oracle.truffle.js.nodes.function.FunctionNameHolder;
 import com.oracle.truffle.js.nodes.function.SetFunctionNameNode;
 import com.oracle.truffle.js.nodes.instrumentation.JSTags;
 import com.oracle.truffle.js.nodes.instrumentation.JSTags.LiteralExpressionTag;
+import com.oracle.truffle.js.nodes.instrumentation.NodeObjectDescriptor;
 import com.oracle.truffle.js.runtime.Errors;
 import com.oracle.truffle.js.runtime.JSContext;
 import com.oracle.truffle.js.runtime.JSRuntime;
@@ -101,7 +103,25 @@ public class ObjectLiteralNode extends JavaScriptNode {
 
     @Override
     public Object getNodeObject() {
-        return JSTags.createNodeObjectDescriptor("type", LiteralExpressionTag.Type.ObjectLiteral.name());
+        NodeObjectDescriptor descriptor = JSTags.createNodeObjectDescriptor("type", LiteralExpressionTag.Type.ObjectLiteral.name());
+        ArrayList<String> memberNames = new ArrayList<>();
+        boolean hasAccessor = false;
+        for (ObjectLiteralMemberNode memberNode : this.members) {
+            String getterSetter = "-";
+            if (memberNode instanceof ObjectLiteralAccessorMemberNode) {
+                hasAccessor = true;
+                getterSetter = (((ObjectLiteralAccessorMemberNode) memberNode).getterNode != null ? "g" : "") + getterSetter;
+                getterSetter = (((ObjectLiteralAccessorMemberNode) memberNode).getterNode != null ? "s" : "") + getterSetter;
+            }
+            if (memberNode instanceof CachingObjectLiteralMemberNode) {
+                memberNames.add(getterSetter + ((CachingObjectLiteralMemberNode) memberNode).name.toString());
+            } else if (memberNode instanceof DictionaryObjectDataMemberNode) {
+                memberNames.add(getterSetter + ((DictionaryObjectDataMemberNode) memberNode).name.toString());
+            }
+        }
+        descriptor.addProperty("members", memberNames.toArray());
+        descriptor.addProperty("hasAccessor", hasAccessor);
+        return descriptor;
     }
 
     public static final class MakeMethodNode extends JavaScriptNode implements FunctionNameHolder.Delegate {

[Haiyang-Sun]

At NodeProf side, I added one extra argument for the literal hook which is an array of the member fields' names according to the definition order. Each name has a prefix delimiter by '-' showing if it is a getter/setter method.

[eleinadani]

OK, thanks for the patch. If possible, I would like to avoid this change in Graal.js, because the data is already available statically (i.e., from the sources).

An alternative implementation that would not require changes to Graal.js might do the following:

1. get the sources from the object literal (e.g., getInstrumentedNode().getSources())
2. re-parse the sources lazily, and get the literal names
3. cache the literal names to avoid re-parsing
4. expose the values
   Parsing at #2 can be done in nodeprof using the Graal.js parser (see com.oracle.js.parser) -- an example of (inline) expression parsing can be found here: https://github.com/graalvm/graaljs/blob/6fe89edae66fec2c61c9512fa91ea2a551ccfbf1/graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/binary/JSTypeofIdenticalNode.java#L183 -- this is more or less the same thing that you would do in Nodeprof to get the literal names, I guess.

Please let me know if this approach works for you.

ps. I am not sure about the hasAccessor flag: why do you want to expose that?

[Haiyang-Sun]

hasAccessor is the flag in the original Jalangi's callback.

I will give it a try, I think using the Graal.js parser should work. I just wonder if it is necessary as such information is already there in the current node, especially when adding such extra information in getNodeObject as above doesn't bring any overhead to Graal.js and will only get called lazily by NodeProf.

[eleinadani]

The general approach is: "everything that can be done without changing the core engine, should be done without changing the core engine" :)

let me know how it goes

[Haiyang-Sun]

@eleinadani it works. As long as the source section bindings are fine, this approach looks good to me.

@mwaldrich The preview of the fix is in #44 and will land soon.

[Haiyang-Sun]

@mwaldrich the PR has been merged, please try if the fix gives you the profiling information you want.