fix: Correctly computes the predecessors weight

Now the predecessors values have the following meaning:
- cumulativeWeight represents the total work in the profile
- weight represents the impact of the callee in the predecessor
  (the impact is the cumulative weight of the callee)

fireplace-app/src/main/java/io/github/bric3/fireplace/jfr/tree/Node.java

@@ -1,3 +1,36 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, Datadog, Inc. All rights reserved.
+ *
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * The contents of this file are subject to the terms of either the Universal Permissive License
+ * v 1.0 as shown at http://oss.oracle.com/licenses/upl
+ *
+ * or the following license:
+ *
+ * Redistribution and use in source and binary forms, with or without modification, are permitted
+ * provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice, this list of conditions
+ * and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright notice, this list of
+ * conditions and the following disclaimer in the documentation and/or other materials provided with
+ * the distribution.
+ *
+ * 3. Neither the name of the copyright holder nor the names of its contributors may be used to
+ * endorse or promote products derived from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
+ * FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
+ * WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
 package io.github.bric3.fireplace.jfr.tree;
 
 import org.openjdk.jmc.flightrecorder.stacktrace.tree.AggregatableFrame;

fireplace-app/src/main/java/io/github/bric3/fireplace/jfr/tree/StacktraceButterflyModel.java

@@ -106,26 +106,27 @@ private static void findPredecessors(
     ) {
         // if there's a match add children nodes
         if (nodeSelector.test(node.getFrame())) {
-            Node predecessorsRootNode = getOrCreateFocusedMethodNode(predecessorsRoot, node.getFrame());
+            Node focusedFrame = getOrCreateFocusedMethodNode(predecessorsRoot, node.getFrame());
 
-            predecessorsRootNode.weight += node.getWeight();
-            predecessorsRootNode.cumulativeWeight += node.getCumulativeWeight();
+            // Only capturing the total of the focused frame (cumulative weight) to
+            // compute the impact on the predecssors
+            double focusedFrameCumulativeWeight = node.getCumulativeWeight();
+            focusedFrame.weight += focusedFrameCumulativeWeight;
+            focusedFrame.cumulativeWeight += focusedFrameCumulativeWeight;
 
             // Adds and merge predecessors in the callers tree
-            Node predecessorNode = predecessorsRootNode;
+            Node currentPredecessor = focusedFrame;
             for (
                     org.openjdk.jmc.flightrecorder.stacktrace.tree.Node currentNode = node.getParent();
-                    currentNode != null && currentNode.getParent() != null;
+                    currentNode != null && !currentNode.isRoot();
                     currentNode = currentNode.getParent()
             ) {
-                if (currentNode.getParent().isRoot()) {
-                    continue;
-                }
-
-                Node child = getOrCreateNode(predecessorNode, currentNode.getFrame());
-                child.weight += currentNode.getWeight();
-                child.cumulativeWeight += currentNode.getCumulativeWeight();
-                predecessorNode = child;
+                Node predecessor = getOrCreateNode(currentPredecessor, currentNode.getFrame());
+                // The amount of weight the focused frame has on predecessors in the back trace
+                predecessor.weight += focusedFrameCumulativeWeight;
+                // The total amount of work done by the predecessors
+                predecessor.cumulativeWeight += currentNode.getCumulativeWeight();
+                currentPredecessor = predecessor;
             }
         }
 
@@ -161,10 +162,10 @@ private static void findSuccessors(
     ) {
         // if there's a match add children nodes
         if (nodeSelector.test(node.getFrame())) {
-            Node successorsRootNode = getOrCreateFocusedMethodNode(successorsRoot, node.getFrame());
-            successorsRootNode.weight += node.getWeight();
-            successorsRootNode.cumulativeWeight += node.getCumulativeWeight();
-            convertAndAddChildren(successorsRootNode, node);
+            Node focusedFrame = getOrCreateFocusedMethodNode(successorsRoot, node.getFrame());
+            focusedFrame.weight += node.getWeight();
+            focusedFrame.cumulativeWeight += node.getCumulativeWeight();
+            convertAndAddChildren(focusedFrame, node);
         }
         // regardless look for matching nodes in children
         for (org.openjdk.jmc.flightrecorder.stacktrace.tree.Node child : node.getChildren()) {
@@ -217,15 +218,18 @@ private static void mergeChildren(Node node) {
         node.children.addAll(childrenMap.values());
     }
 
-    private static void convertAndAddChildren(Node successorsRootNode, org.openjdk.jmc.flightrecorder.stacktrace.tree.Node node) {
+    private static void convertAndAddChildren(Node successorsParentNode, org.openjdk.jmc.flightrecorder.stacktrace.tree.Node node) {
         List<Node> list = new ArrayList<>();
         for (org.openjdk.jmc.flightrecorder.stacktrace.tree.Node child : node.getChildren()) {
-            Node jmcTreeNode = toJmcTreeNode(successorsRootNode, child);
+            Node jmcTreeNode = toJmcTreeNode(successorsParentNode, child);
             list.add(jmcTreeNode);
         }
-        successorsRootNode.children.addAll(list);
+        successorsParentNode.children.addAll(list);
     }
 
+    /*
+     * Unfortunately it is required to convert the nodes since this class live in a different package.
+     */
     private static Node toJmcTreeNode(Node convertedParent, org.openjdk.jmc.flightrecorder.stacktrace.tree.Node node) {
         Node converted = new Node(
                 convertedParent,
@@ -238,10 +242,31 @@ private static Node toJmcTreeNode(Node convertedParent, org.openjdk.jmc.flightre
         return converted;
     }
 
+    /**
+     * Returns the root node of the successors tree.
+     * The successors tree is a regular flamegraph whose root is the focused frame.
+     * <ul>
+     *     <li>weight: the <em>self</em> value</li>
+     *     <li>cumulativeWeight: the sum of work done by this frame and it's callees</li>
+     *     <li>children: the callees</li>
+     * @return the root node of the successors tree
+     */
     public Node getSuccessorsRoot() {
         return successorsRoot;
     }
 
+    /**
+     * Returns the root node of the predecessors tree.
+     * The predecessors tree represents merged callers of the focused method,
+     * the node values in the tree have a slightly different meaning than a regular flamegraph:
+     * <ul>
+     *     <li>weight: the amount of weight the focused frame has on predecessors</li>
+     *     <li>cumulativeWeight: the total weight of work done by the predecessors in the whole profile</li>
+     *     <li>children: the merged predecessors</li>
+     *     <li>parent: the callee</li>
+     * </ul>
+     * @return the root node of the predecessors tree
+     */
     public Node getPredecessorsRoot() {
         return predecessorsRoot;
     }

fireplace-app/src/main/kotlin/io/github/bric3/fireplace/jfr/support/JfrFrameNodeConverter.kt

@@ -11,6 +11,7 @@ package io.github.bric3.fireplace.jfr.support
 
 import io.github.bric3.fireplace.flamegraph.FrameBox
 import org.openjdk.jmc.flightrecorder.stacktrace.tree.Node
+import java.util.function.ToDoubleFunction
 
 /**
  * Creates an array of FlameNodes that live in the [0.0, 1.0] world space on the X axis and the depth of the stack representing
@@ -35,20 +36,25 @@ object JfrFrameNodeConverter {
         return nodes
     }
 
-    // Compatibility with Node duplicata
-    fun convert(node: io.github.bric3.fireplace.jfr.tree.Node): List<FrameBox<io.github.bric3.fireplace.jfr.tree.Node>> {
+    fun convertButterfly(
+        node: io.github.bric3.fireplace.jfr.tree.Node,
+        nodeWeightFunction: ToDoubleFunction<io.github.bric3.fireplace.jfr.tree.Node>
+    ): MutableList<FrameBox<io.github.bric3.fireplace.jfr.tree.Node>> {
         val nodes = mutableListOf<FrameBox<io.github.bric3.fireplace.jfr.tree.Node>>()
         FrameBox.flattenAndCalculateCoordinate(
             nodes,
             node,
             io.github.bric3.fireplace.jfr.tree.Node::getChildren,
-            io.github.bric3.fireplace.jfr.tree.Node::getCumulativeWeight,
-            { it.children.stream().mapToDouble(io.github.bric3.fireplace.jfr.tree.Node::getCumulativeWeight).sum() },
+            nodeWeightFunction,
+            { it.children.stream().mapToDouble(nodeWeightFunction).sum() },
             0.0,
             1.0,
             0
         )
         assert(nodes[0].actualNode.isRoot) { "First node should be the root node" }
         return nodes
     }
+
+    fun predecessorsWeight() = io.github.bric3.fireplace.jfr.tree.Node::getWeight
+    fun successorsWeight() = io.github.bric3.fireplace.jfr.tree.Node::getCumulativeWeight
 }

fireplace-app/src/main/kotlin/io/github/bric3/fireplace/ui/ButterflyPane.kt

@@ -15,7 +15,9 @@ import io.github.bric3.fireplace.core.ui.Colors
 import io.github.bric3.fireplace.flamegraph.*
 import io.github.bric3.fireplace.jfr.support.JfrFrameColorMode
 import io.github.bric3.fireplace.jfr.support.JfrFrameColorMode.BY_PACKAGE
-import io.github.bric3.fireplace.jfr.support.JfrFrameNodeConverter
+import io.github.bric3.fireplace.jfr.support.JfrFrameNodeConverter.convertButterfly
+import io.github.bric3.fireplace.jfr.support.JfrFrameNodeConverter.predecessorsWeight
+import io.github.bric3.fireplace.jfr.support.JfrFrameNodeConverter.successorsWeight
 import io.github.bric3.fireplace.jfr.tree.Node
 import io.github.bric3.fireplace.jfr.tree.StacktraceButterflyModel
 import io.github.bric3.fireplace.ui.toolkit.BalloonToolTip
@@ -145,8 +147,8 @@ class ButterflyPane : JPanel(BorderLayout()) {
     }
 
     private fun dataApplier(stacktraceButterflyModel: StacktraceButterflyModel): Consumer<ButterflyView<Node>> {
-        val predecessorsFlatFrameList = JfrFrameNodeConverter.convert(stacktraceButterflyModel.predecessorsRoot)
-        val successorsFlatFrameList = JfrFrameNodeConverter.convert(stacktraceButterflyModel.successorsRoot)
+        val predecessorsFlatFrameList = convertButterfly(stacktraceButterflyModel.predecessorsRoot, predecessorsWeight())
+        val successorsFlatFrameList = convertButterfly(stacktraceButterflyModel.successorsRoot, successorsWeight())
         val title = stacktraceButterflyModel.focusedMethod.method.methodName
         return Consumer { flameGraph ->
             val frameEquality: (a: FrameBox<Node>, b: FrameBox<Node>) -> Boolean =
@@ -246,9 +248,9 @@ class ButterflyPane : JPanel(BorderLayout()) {
                     append(frame.actualNode.frame.humanReadableShortString)
                     append("</b><br>")
                     append(desc)
-                    append("<br><hr>")
+                    append("<br><hr>Cumulative weight:")
                     append(frame.actualNode.cumulativeWeight)
-                    append(" ")
+                    append(" Weight:")
                     append(frame.actualNode.weight)
                     append("<br>BCI: ")
                     append(frame.actualNode.frame.bci ?: "N/A")