Upgrade D3 and d3-flameGraph #216
Conversation
webroot/css/xhgui.css
Outdated
| background: #fff; | ||
| border-radius: 6px; | ||
| box-shadow: 0 5px 10px rgba(0, 0, 0, 0.2); | ||
| border: 1px solid rgba(0, 0, 0, 0.2); |
There was a problem hiding this comment.
Fallback border (hex or RGB) should precede RGBA border.
There was a problem hiding this comment.
(pre-existing issue, unrelated)
|
When testing on this branch I notice that these upstream improvements expose a pre-existing bug in XHGui. First, the upstream improvements:
This second point, means the size of nodes in the flame graph is now accurate, and the hover tip shows the correct microsecond values now. However, this given the width of the rectangles now being computed directly based on the node's own value relative to In current master this data corruption is not obvious at first because flameGraph computes the width based on their children, which means "Something\here" would (wrongly) be given Instead, on this pull requests' branch, the graph can now be rendered in a way that has various rectangles overlapping each other because the children can sometimes be wider than their parent:
Note in the after image how some of the rectangles now have rectangles transparently behind them. |
This ensures a test exists for cases where there are multiple call stacks that have the same function in their leaf position. This is a known use case that XHGui accounts with its calculateSelf() method, but it seems the current getFlameGraphData() method uses this data incorrectly. This issue is mentioned at perftools#216 (comment) Before I fix this, I'm first adding a test to show the current broken behaviour.
This ensures a test exists for cases where there are multiple call stacks that have the same function in their leaf position. This is a known use case that XHGui accounts with its calculateSelf() method, but it seems the current getFlameGraphData() method uses this data incorrectly. This issue is mentioned at perftools#216 (comment) Before I fix this, I'm first adding a test to show the current broken behaviour.
This ensures a test exists for cases where there are multiple call stacks that have the same function in their leaf position. This is a known use case that XHGui accounts with its calculateSelf() method, but it seems the current getFlameGraphData() method uses this data incorrectly. This issue is mentioned at perftools#216 (comment) Before I fix this, I'm first adding a test to show the current broken behaviour.
| @@ -72,12 +70,6 @@ d3.json("{{ url('run.flamegraph.data', {id: profile.id|trim }) }}", function(err | |||
| return console.warn(error); | |||
| } | |||
|
|
|||
| flameGraph.height(getDepth(data.data) * cellHeight); | |||
|
|
|||
| flameGraph.sort(function (a, b) { | |||
There was a problem hiding this comment.
Note to self: This is actually not redundant. Due to the code accessing data.name I mistakenly thought this is a name sort, but it's actually looking in XHGui's data object, not a D3 d.data object.
This is from fb8fac5 and needs to stay.
|
The children being wider than the parents results in some very funny looking graphs locally. I'll see if I can get the calculations behind the flamegraph to add up better. The tests you contributed in #217 will be helpful in verifying any fixes. |
### d3.flameGraph Originally added in May 2016 as: - d3.flameGraph [v0.4.2] - d3.tip [v0.6.7] (its dependency) At that time, d3.flameGraph required v3.x of d3, which XHGui already bundled. The latest version of d3.flameGraph requires v4.x of d4. Update to: - d3.flameGraph [v1.0.11-dev] - d3.tip [v0.7.1] (its dependency) Preserving two patches: - d3.flameGraph.js: Override offset to 24px for d3.tip(). - d3.flameGraph.css: Omit d3-flame-graph-tip styles, we have our own. Change inline script to account for upstream changes: * Remove redundant 'height' override (matches the default: depth * cellHeight). * Remove redundant 'sort' override (matches the default: by name ASC). * Set digits=0 for Xhgui.formatNumber() instead of default digits=2, given that the digits are always ".00". This matches the way microsecond values are rendered elsewhere in XHGui. ## d3.js Upgrade to 4.x (v4.0.8 exactly, same as d3.flameGraph uses now). Apply changes required for D3 version 4 per <https://github.com/d3/d3/blob/v4.6.0/CHANGES.md> ### xhgui-charts.js * d3.layout.pie > d3.pie * d3.time.format > d3.timeFormat ### flamegraph.twig * 'cubic-in-out' > d3.easeCubic * tip() html callback: 'name' and 'value are now in d.data. * tip() html callback: d.dx replaced with d.x0 and d.x1
|
@markstory I see the tests were merged, but I'm unsure as to whether the problem was solved. Do all nodes now have values that are never less than the total of direct children? Let me know if/when it is resolved, or what I can do to help, would be nice to move this along. While the master version (last I checked) didn't appear broken, as mentioned in the previous comments, it is also broken due to the nodes being recursively summed. This has two notable side-effects: Proportions are off (small values are shown too small, large values are shown too large), and the total is way too high. However, it does have the benefit of masking the problem we found in this pull request. |
|
I don't think the root problem was solved. Last I remember there were different display issues with these changes. I haven't had much time to dedicate to this project as of late though. |
This unfortunately never worked correctly due to a fundamental limitation with the XHProf data format, which is that it only records metadata per parent-child method combination, it cannot be used to build a complete call tree. The feature was added in pull perftools#177. More information about this limitation is described in detail at perftools#219, perftools#216 and perftools#212. A brief summary: * The visualisation showed callstacks that did not actually exist, and was missing callstacks that did exist. (Due to assuming that every combination of a parent-child pair is valid, and due to it randomly assinging repeated calls to the first encountered parent.) * The visualisation discarded all timing values from XHProf, except for the timing of leaf nodes (methods without children), which were then added up recursively. The end result was a visually well-balanced tree, but with timing values that were not related to the actual performance (upto 100x inflated), and the proportions were incorrect as well, making some code look fast instead of slow, and vice versa. These are inherent problems that cannot be solved because the information logically required to make a flamegraph (call stacks) is not collected by XHProf. This closes perftools#216, perftools#212, perftools#211, perftools#207. This fixes perftools#212.
This unfortunately never worked correctly due to a fundamental limitation with the XHProf data format, which is that it only records metadata per parent-child method combination, it cannot be used to build a complete call tree. The feature was added in pull perftools#177. More information about this limitation is described in detail at perftools#219, perftools#216 and perftools#212. A brief summary: * The visualisation showed callstacks that did not actually exist, and was missing callstacks that did exist. (Due to assuming that every combination of a parent-child pair is valid, and due to it randomly assinging repeated calls to the first encountered parent.) * The visualisation discarded all timing values from XHProf, except for the timing of leaf nodes (methods without children), which were then added up recursively. The end result was a visually well-balanced tree, but with timing values that were not related to the actual performance (upto 100x inflated), and the proportions were incorrect as well, making some code look fast instead of slow, and vice versa. These are inherent problems that cannot be solved because the information logically required to make a flamegraph (call stacks) is not collected by XHProf. This closes perftools#216, perftools#212, perftools#211, perftools#207. This fixes perftools#212.
|
Closing in favour of #246. |
This unfortunately never worked correctly due to a fundamental limitation with the XHProf data format, which is that it only records metadata per parent-child method combination, it cannot be used to build a complete call tree. The feature was added in pull perftools#177. More information about this limitation is described in detail at perftools#219, perftools#216 and perftools#212. A brief summary: * The visualisation showed callstacks that did not actually exist, and was missing callstacks that did exist. (Due to assuming that every combination of a parent-child pair is valid, and due to it randomly assinging repeated calls to the first encountered parent.) * The visualisation discarded all timing values from XHProf, except for the timing of leaf nodes (methods without children), which were then added up recursively. The end result was a visually well-balanced tree, but with timing values that were not related to the actual performance (upto 100x inflated), and the proportions were incorrect as well, making some code look fast instead of slow, and vice versa. These are inherent problems that cannot be solved because the information logically required to make a flamegraph (call stacks) is not collected by XHProf. This closes perftools#212, perftools#211, perftools#207. This fixes perftools#212.
This builds on top of #215.
The upgrade is a fairly large commit because D3 and d3-flameGraph have to be updated at the same time, and they bring with them a few breaking changes that required the frontend glue in
flamegraph.twigandxhgui-charts.jsto be changed a little bit.