Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Legibility issues on flame graph #211

Closed
Krinkle opened this issue Oct 16, 2017 · 3 comments
Closed

Legibility issues on flame graph #211

Krinkle opened this issue Oct 16, 2017 · 3 comments

Comments

@Krinkle
Copy link
Contributor

Krinkle commented Oct 16, 2017

On a typical flame graph I find two problems that impair its usability:

screen shot 2017-10-16 at 16 44 14

  1. The text has a text-shadow applies which makes the text somewhat blurry and rather hard to read.
  2. The color contrast between the yellow, orange and reds is very low. The colors look almost identical.

Regarding the text-shadow, I believe this was unintentionally inherited from Bootstrap styles. When inspecting one of the labels, I find it has a class="label" and the D3 styles happen to overlap almost all the styles inherited from Boostrap, but not the text-shadow, that one survives. Given that grey shadow was meant for white text originally within Bootstrap, it is now applying to black text and essentially producing a blur.

.d3-flame-graph .label { – from d3.flameGraph

  • pointer-events: none;
  • white-space: nowrap;
  • text-overflow: ellipsis;
  • overflow: hidden;
  • font-size: 12px;
  • font-family: Verdana;
  • margin-left: 4px;
  • margin-right: 4px;
  • line-height: 1.5;
  • padding: 0 0 0;
  • font-weight: 400;
  • color: black;
  • text-align: left;

.label, .badge { – from bootstrap.min.css

  • display: inline-block;
  • padding: 2px 4px;
  • font-size: 11.844px;
  • font-weight: bold;
  • line-height: 14px;
  • color: #ffffff;
  • vertical-align: baseline;
  • white-space: nowrap;
  • text-shadow: 0 -1px 0 rgba(0, 0, 0, 0.25);
  • background-color: #999999;

Regarding the flame color contrast, would be good to align closer with Brendan Gregg's original color scheme:

d3.flameGraph brendangregg/FlameGraph
screen shot 2017-10-16 at 16 44 14 screen shot 2017-10-16 at 16 51 11
@Krinkle
Copy link
Contributor Author

Krinkle commented Oct 16, 2017

Regarding the text rendering issue, to confirm - the original demo is not affected http://martinspier.io/d3-flame-graph/

This is purely a problem from D3 and Bootstrap ending up on the same page. We could override the styles stronger on our side. But a better solution would be for upstream to use a less generic class name in this case.

=> Upstream: spiermar/d3-flame-graph#85

@markstory
Copy link
Member

Any chance you'd be willing to make a pull request with with the proposed changes?

Krinkle added a commit to Krinkle/d3-flame-graph that referenced this issue Nov 10, 2017
To avoid conflicts with Bootstrap's "label" class, as used by
the XHGui project. Upstream issue: perftools/xhgui#211.

Follows-up 9d482c1 and spiermar#38.

Fixes spiermar#85.
@Krinkle
Copy link
Contributor Author

Krinkle commented Nov 12, 2017

Note to self:

Updating d3.flameGraph CSS/JS to latest versions is blocked on:

  • Deal with local hack 1: 71f521e
  • Deal with local hack 2: 0ff9af8
  • Upgrade d3 from v3.x to v4.x. (Current: 3.0.8, Previously required by flameGraph: 3.5, now required by flameGraph: 4.0)

First error I got on current d3 version is d3.partition undefined. Looks like it moved from d3.layout.partition to d3.partition.

Krinkle added a commit to Krinkle/xhgui that referenced this issue Nov 13, 2017
Follows-up 71f521e.

* Undo the overrides and place them in xhgui.css instead.
* Add a class "xhgui-flamegraph" to the main chart element
  so that we can uniquely address flamegraphs within xhgui
  with a higher CSS specificity selector (using the same selector
  as in d3.flameGraph.css won't work given that they have the
  same weight, and load order would break the tie, which currently
  means the upstream one applies last, use a stronger selector
  instead of relying on load order).

This is in preparation of upgrading d3.flameGraph to a newer
version from upstream.

Ref perftools#211.
Krinkle added a commit to Krinkle/xhgui that referenced this issue Nov 13, 2017
Follows-up 0ff9af8.

* Move the overrides to xhgui.css instead.
* Given the complexity of these overrides, don't try to undo
  each original style, but simply document that all
  d3-flame-graph-tip styles should be omitted when updating.
* Document the d3.flameGraph.js patch.

Ref perftools#211.
Krinkle added a commit to Krinkle/xhgui that referenced this issue Nov 13, 2017
Follows-up 0ff9af8.

* Move the overrides to xhgui.css instead.
* Given the complexity of these overrides, don't try to undo
  each original style, but simply document that all
  d3-flame-graph-tip styles should be omitted when updating.
* Document the d3.flameGraph.js patch.

* Add ignore for pre-existing issues with csslint. Since
  sticker-ci was enabled without passing on current code first.
  Also, these issues are false negatives, and are bugs in csslint,
  not bugs in xhgui code.

Ref perftools#211.
Krinkle added a commit to Krinkle/xhgui that referenced this issue Nov 16, 2017
Follows-up 71f521e.

* Undo the overrides and place them in xhgui.css instead.
* Add a class "xhgui-flamegraph" to the main chart element
  so that we can uniquely address flamegraphs within xhgui
  with a higher CSS specificity selector (using the same selector
  as in d3.flameGraph.css won't work given that they have the
  same weight, and load order would break the tie, which currently
  means the upstream one applies last, use a stronger selector
  instead of relying on load order).

This is in preparation of upgrading d3.flameGraph to a newer
version from upstream.

Ref perftools#211.
Krinkle added a commit to Krinkle/xhgui that referenced this issue Nov 16, 2017
Follows-up 0ff9af8.

* Move the overrides to xhgui.css instead.
* Given the complexity of these overrides, don't try to undo
  each original style, but simply document that all
  d3-flame-graph-tip styles should be omitted when updating.
* Document the d3.flameGraph.js patch.

* Add ignore for pre-existing issues with csslint. Since
  sticker-ci was enabled without passing on current code first.
  Also, these issues are false negatives, and are bugs in csslint,
  not bugs in xhgui code.

Ref perftools#211.
lichunqiang pushed a commit to lichunqiang/xhgui that referenced this issue Dec 28, 2017
Follows-up 71f521e.

* Undo the overrides and place them in xhgui.css instead.
* Add a class "xhgui-flamegraph" to the main chart element
  so that we can uniquely address flamegraphs within xhgui
  with a higher CSS specificity selector (using the same selector
  as in d3.flameGraph.css won't work given that they have the
  same weight, and load order would break the tie, which currently
  means the upstream one applies last, use a stronger selector
  instead of relying on load order).

This is in preparation of upgrading d3.flameGraph to a newer
version from upstream.

Ref perftools/xhgui#211.
Krinkle added a commit to Krinkle/xhgui that referenced this issue Sep 23, 2018
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.
Krinkle added a commit to Krinkle/xhgui that referenced this issue Sep 23, 2018
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.
Krinkle added a commit to Krinkle/xhgui that referenced this issue Sep 23, 2018
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.
@Krinkle Krinkle closed this as completed Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants