1117825 - Create an utility function for the FlameGraph widget to convert the profiler data into something drawable

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8544059 [details] [diff] [review]
v1

Review of attachment 8544059 [details] [diff] [review]:
-----------------------------------------------------------------

r+

::: browser/devtools/shared/test/browser_flame-graph-utils.js
@@ +4,5 @@
> +// Tests that text metrics in the flame graph widget work properly.
> +
> +let {FlameGraphUtils} = Cu.import("resource:///modules/devtools/FlameGraph.jsm", {});
> +
> +let test = Task.async(function*() {

Should use `add_task(function*(){})`

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8544059 [details] [diff] [review]
v1

Also this.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8544059 [details] [diff] [review]
v1

Review of attachment 8544059 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/widgets/FlameGraph.jsm
@@ +738,5 @@
>  
> +const FLAME_GRAPH_BLOCK_HEIGHT = 12;
> +
> +const PALLETTE_SIZE = 10;
> +const PALLETTE_HUE_OFFSET = Math.random() * 90;

Are you sure you want Math.random()? I see no benefit of not being consistent here.

@@ +839,5 @@
> +   *
> +   * @param string src
> +   * @return number
> +   */
> +  _getStringSalt: function(src) {

salt != hash

Not that a hash collision would be bad at all here it's easy to get a significantly better hash function. Instead of summing, multiply by a prime number. This will make it avoid simple, accidental and non malicious, collision from occurring for similar words:
http://stackoverflow.com/a/2624210

Again, not critical here but it's simple and a good habit.

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Benoit Girard (:BenWa) from comment #4)
> 
> Not that a hash collision would be bad at all here it's easy to get a
> significantly better hash function. Instead of summing, multiply by a prime
> number. This will make it avoid simple, accidental and non malicious,
> collision from occurring for similar words:
> http://stackoverflow.com/a/2624210
> 
> Again, not critical here but it's simple and a good habit.

Good idea.

Comment 6

[Victor Porof [:vporof][:vp]]

Addressed comments and landed: https://hg.mozilla.org/integration/fx-team/rev/db5eef36bcd9

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for dt2 orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=1645879&repo=fx-team
https://hg.mozilla.org/integration/fx-team/rev/ba0cafaf462d

Comment 8

[Victor Porof [:vporof][:vp]]

Trying again: https://hg.mozilla.org/integration/fx-team/rev/f919a460dc30

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f919a460dc30