1164252 - Profiler should be able to parse evaluated location sites

Status: RESOLVED FIXED

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

Description

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

These eval'd frames appear in octane tests, and are not parsed properly.

".l (http://localhost:8888/octane-benchmark/zlib-data.js line 65 > eval:1)"
"http://localhost:8888/octane-benchmark/zlib-data.js line 65 > eval:1"

Comment 1

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

By not parsed properly, I mean the file name is "zlib-data%20line%65%20>%20eval:1", and same with the full URL, with the line 65 > eval:1 stuff being a part of it.

Comment 2

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

The function `parseLocation` in `gecko/devtools/client/performance/modules/logic/frame-utils.js` needs to handle the scenario of parsing a frame location of

".l (http://localhost:8888/octane-benchmark/zlib-data.js line 65 > eval:1)"

and the fileName returned should be "zlib-data.js line 65 > eval:1" -- should be escaped.

Tests for this function can be found at `devtools/client/performance/test/unit/test_frame-utils-01.js`

Comment 3

[Greg Tatum [:gregtatum]]

Attachment: Bug1164252.patch

Let's see if I did this patch correctly.

Comment 4

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

Comment on attachment 8712761 [details] [diff] [review]
Bug1164252.patch

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

Looks good! Some comments on readability and performance and assertions -- r? again once these changes are made and I'll send this up to try. Thanks!

::: devtools/client/performance/modules/logic/frame-utils.js
@@ +151,5 @@
>      host = parsedUrl.host;
> +
> +    // Check for the case of the filename containing eval
> +    // e.g. "file.js%20line%2065%20%3E%20eval"
> +    if (fileName.substring(fileName.length - 13, fileName.length) === "%20%3E%20eval") {

We can store a constant (can be at the top of the file too, with all the others) so we don't have a magic number 13 here, as well as stick to comparing numbers without generating new strings (this piece of code is still hot and the conditional will get hit often) -- the fastest way would put this in a function and check the char codes like other functions in this file, but this way is more legible and I'm sure fast enough (should run some tests though)

```
const EVAL_TOKEN = "%20%3E%20eval";
if (fileName.indexOf(EVAL_TOKEN) === (fileName.length - EVAL_TOKEN.length)) {
```

I'm sure there's an off by one error in my code snippet

@@ +153,5 @@
> +    // Check for the case of the filename containing eval
> +    // e.g. "file.js%20line%2065%20%3E%20eval"
> +    if (fileName.substring(fileName.length - 13, fileName.length) === "%20%3E%20eval") {
> +      // Match the filename
> +      let matches = fileName.match(/(.+)(%20line%20(\d+)%20%3E%20eval)/);

We can use destructuring here to provide a bit more clarity:

`let [_, _fileName, _, _line] = fileName.match(/(.+)(%20line%20(\d+)%20%3E%20eval)/) || [];`

In what scenario would the EVAL_TOKEN is found, but no matches occur? Profiler data is always a fun surprise, so good to play it safe/defensive here even if we always assume a scenario, but let's log something incase this isn't true at some point in the future.

Check out the `assert` function from `devtools/shared/DevToolsUtils` which can log an error if `!matches === true`

@@ +161,5 @@
> +        line = matches[3];
> +      }
> +
> +      // Match the url as well
> +      matches = url.match(/(.+)( line (\d+) > eval)/);

Same thing as the above matching; log an error if this assumption is ever untrue with assert

Comment 5

[Greg Tatum [:gregtatum]]

Attachment: Bug1164252.patch

Comment 6

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

Comment on attachment 8712840 [details] [diff] [review]
Bug1164252.patch

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

Looks good! Pushing to try: 
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6016324b010

Comment 7

[Greg Tatum [:gregtatum]]

Attachment: Bug1164252.patch

let evalIndex = fileName.indexOf(EVAL_TOKEN);
if (evalIndex !== -1 && evalIndex === (fileName.length - EVAL_TOKEN.length))

I wasn't checking for the case that the EVAL_TOKEN wasn't found.

Comment 8

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

Comment on attachment 8713228 [details] [diff] [review]
Bug1164252.patch

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8894a4f80a3

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3111dc2fd952

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3111dc2fd952