Closed Bug 1117825 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file)

No description provided.
Blocks: perf-tool-v2
Depends on: 1077444
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
Attachment #8544059 - Flags: review?(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*(){})`
Attachment #8544059 - Flags: review?(jsantell) → review+
Comment on attachment 8544059 [details] [diff] [review]
v1

Also this.
Attachment #8544059 - Flags: review?(bgirard)
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.
Attachment #8544059 - Flags: review?(bgirard) → review+
(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.
Addressed comments and landed: https://hg.mozilla.org/integration/fx-team/rev/db5eef36bcd9
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f919a460dc30
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.