refactor: html elements by spiermar · Pull Request #124 · spiermar/d3-flame-graph

spiermar · 2018-10-23T21:31:19Z

No description provided.

This is a large change that ripples through many parts of the project. Motivation behind this change: - Desire for faster and smoother animations - Shorter page load times - Shorter zoom in/out delay - Make nodes text selectable - Make tooltip text selectable This was achieved via following changes: - Switch from svg to HTML elements. This simplifies tree structure and speeds up layout and rendering (each svg group had `rect` and `foreignObject` with `div` inside, now we have just a single `div` for each node). - Switch from d3 animations to CSS transitions (smoother, less overhead) - Hold `Shift` while tooltip is up to keep it on screen (allows to select tooltip text) - Don't activate `zoom()` when text is selected on page (allows to select node text) - Use CSS style for search highlight and default to border color change (preserves background color that reflects regressions) - Adaptive tooltip positioning (allows larger tooltips and makes sure that entire tooltip is visible in browser's client area) - Don't use ideomatic d3 attr setters (while nice looking, they are slower than more conventional approach) Conflicts: dist/d3-flamegraph.js dist/d3-flamegraph.min.js

Conflicts: dist/d3-flamegraph.min.js

Otherwise zoom-in will go out of container bounds. Conflicts: dist/d3-flamegraph.min.js

This way flamegraph will be as wide as containing element. E.g. you can resize browser window, call update() and flamegraph will fill available space. Conflicts: dist/d3-flamegraph.min.js

Conflicts: dist/d3-flamegraph.min.js

spiermar · 2018-10-23T21:31:27Z

@wonder-mice that's the large one. Might need a bit more time to review. After this, it's just a simple function rename PR that should be merged straight away.

spiermar · 2018-10-23T22:33:46Z

Without looking into the code too much yet, with my not-so-large example profiles, this can be 2x faster than current master. Having said that, it's a very different approach to rendering the data, and the styling needs to be improved a bit. The colors seem to be off, a bit darker, and the highlighted nodes are not easily visible.
I might cherry pick a couple other changes from this PR, like the tooltips, before reviewing the code in more detail.

wonder-mice · 2018-10-23T23:14:32Z

Colors are darker because I removed transparency to make blending easier for browser. Color mapper should care about that, imho. That said, I talked with WebKit folks and they said it doesn’t really matters, since it always does alpha blending regardless. Still think opacity by default is bad :)

spiermar

Overall, the bulk of the improvement is coming from not using SVG elements. My comments are as follow:

set opacity back to default. Ok with having an option to disable it.
highlight the background of the frame instead of the border. Ok with configurable color.
liked the new tooltips, specially removing a dependency. I would move the tooltip portion of the PR to a separate PR, specially because of the document event listeners.
update the d3 calls to use chaining.
move the initial createElement to d3 calls.
transitionDuration and transitionEase functions added back

spiermar · 2018-10-23T23:16:41Z

  var maxDelta = 0
+  var p = partition()
+
+  const containerElement = document.createElement('div')


This is an anti-pattern on d3 plugins. Usually you would want to use something similar to .append('div') on .enter().

spiermar · 2018-10-23T23:27:57Z

+  containerElement.appendChild(titleElement)
+  containerElement.appendChild(nodesSpaceElement)
+
+  const externalState = {


This is being used keep the tooltip in the same place and selecting text only, correct? If that's the case, I would try to move that to a separate feature pull request. Not a big fan of adding document event listeners and handlers in the plugin. There might be a way of keeping the key state in an external object.

Yeah, I didn't want to do it either, but didn't find any other way...

I can't think of a solution right now, that's why I suggested taking a separate PR to deal with it.

spiermar · 2018-10-23T23:33:16Z

    detailsElement.innerHTML = searchSum + ' of ' + totalValue + ' samples ( ' + format('.3f')(100 * (searchSum / totalValue), 3) + '%)'
  }

+  var classMapper = function (d, base) {


Can probably use multiple .attr("class", d) in the d3 chain.

spiermar · 2018-10-23T23:37:09Z

+  }
+
  var colorMapper = function (d) {
-    return d.highlight ? '#E600E6' : colorHash(getName(d), getLibtype(d), getDelta(d))


Setting a class and using CSS to set the color of the highlights. Is there a reason why the CSS only sets the border color and not the background color?

I choose such CSS style because I wanted to preserve bg color. I only use it in differential mode and in this mode colors are very important.

I just find it a bit hard to visualize the highlighted frames with just a different border color.

spiermar · 2018-10-23T23:44:33Z

+
+    // EXIT old elements not present in new data.
+    g.exit().each(function (d) {
+      this.style.display = 'none'


Could use chain here too. .style('display', 'none')

Chains are slower. They loop over collection on each chain "link" and do other unnecessary things.

Overhead was never significant, but I can profile again.

spiermar · 2018-10-23T23:45:18Z

+    // UPDATE old elements present in new data.
+    g.each(function (d) {
+      const wpx = width(d)
+      this.className = classMapper(d, wpx < 35 ? 'node-sm' : 'node')


spiermar · 2018-10-23T23:49:33Z

-    return chart
-  }
-
-  chart.transitionEase = function (_) {


Some users depend on these functions. I'm Ok with not having it as a default, but option should be there.

Well, problem is that now transitions are CSS based and I don't see how to keep these functions "functional", e.i. I can see how we can keep them, but they will not work.

We have a great D3 guy here in the office, so I'll ping him see if he has any ideas.

wonder-mice · 2018-10-24T15:33:57Z

I realized my mistake. Looks like your intention was to be a d3 plugin. But I was looking at it as flamegraph builder that just happened to use d3 because it was easier at some point. I have some pending patches that add very interesting functionality (better comparison mode), but they have d3 completely removed (because I didn't find how to do it with d3 efficiently and the only part of d3 still in use was node match-by-id-reuse machinery). I surely share the patches later, maybe you'll figure out how to do it in d3 if you find them interesting enough.

spiermar · 2018-10-24T20:22:24Z

I realized my mistake. Looks like your intention was to be a d3 plugin. But I was looking at it as flamegraph builder that just happened to use d3 because it was easier at some point. I have some pending patches that add very interesting functionality (better comparison mode), but they have d3 completely removed (because I didn't find how to do it with d3 efficiently and the only part of d3 still in use was node match-by-id-reuse machinery). I surely share the patches later, maybe you'll figure out how to do it in d3 if you find them interesting enough.

Not particularly attached to d3, but I've built other pure JavaScript visualizations in the past and quickly things get out of hand, and I've found d3 to be easier to maintain in the long run. I'll try to get all these changes in first, then we can chat about the other changes.
I can have a look in the patches and talk with a few guys here about possible d3 implementation ideas too.

Andrey Pokrovskiy added 7 commits October 23, 2018 13:37

fix: don't replace user provided datum with hierarchy nodes

65030b6

Conflicts: dist/d3-flamegraph.min.js

fix: don't recalculate height after node filtering

f23e925

Otherwise zoom-in will go out of container bounds. Conflicts: dist/d3-flamegraph.min.js

feat: match container's width by default

0a6fa83

This way flamegraph will be as wide as containing element. E.g. you can resize browser window, call update() and flamegraph will fill available space. Conflicts: dist/d3-flamegraph.min.js

fix: y-positioning (off by 1 error)

85484e9

Conflicts: dist/d3-flamegraph.min.js

fix: inject fade and hide props into nodes, not data

110059b

Conflicts: dist/d3-flamegraph.min.js

fix: update cell background color too, since cell elements are reused

1191bb0

Conflicts: dist/d3-flamegraph.min.js

fix: removing removed functions from examples

98d9a4c

spiermar commented Oct 23, 2018

View reviewed changes

Conversation

spiermar commented Oct 23, 2018

Uh oh!

spiermar commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spiermar commented Oct 23, 2018

Uh oh!

wonder-mice commented Oct 23, 2018

Uh oh!

spiermar left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wonder-mice commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spiermar commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spiermar commented Oct 23, 2018 •

edited

Loading

wonder-mice commented Oct 24, 2018 •

edited

Loading