1226319 - [JIT View] Clean up everything

Status: RESOLVED FIXED

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

Description

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

This view is my garbage mess prototype hack. Let's clean up the code completely, use the tree view from the memory tool, and make it possible to make changes/features without being terrible.

Comment 1

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

Attachment: 1226319-1-jit-refactor.patch

First pass for moving tree component to shared, and using browser loader for perf

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8689685 [details] [diff] [review]
1226319-1-jit-refactor.patch

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

Nice

::: devtools/client/shared/components/test/mochitest/head.js
@@ +27,5 @@
>  var { TargetFactory } = require("devtools/client/framework/target");
>  var { Toolbox } = require("devtools/client/framework/toolbox");
>  
>  DevToolsUtils.testing = true;
> +var { require: bRequire } = BrowserLoader("resource://devtools/client/shared/", this);

s/bRequire/browserRequire/

Comment 3

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

First patch is only going to handle moving the memory tree. Perf tool is going to take some more changing to work with browserloader, as the "modules" currently all access each other from global scope, yuckily enough.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3929c7bc30e8

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3929c7bc30e8

Comment 6

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

Attachment: 1226319-2-frame-node.patch

Pulling out pieces to land this now... changes to source frame

Comment 7

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

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

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8710901 [details] [diff] [review]
1226319-2-frame-node.patch

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

::: devtools/client/locales/en-US/shared-components.properties
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

Maybe just name this "components.properties"? *shrug*

::: devtools/client/shared/components/frame.js
@@ +25,5 @@
>      onClick: PropTypes.func.isRequired,
> +    // Option to display a function name before the source link.
> +    showFunctionName: PropTypes.bool,
> +    // Option to display a host name after the source link.
> +    showHost: PropTypes.bool,

I think we should add a `getDefaultProps` method that returns `true` or `false` for these optional properties.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/86a6ad97f0f5

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/86a6ad97f0f5

Comment 11

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

Attachment: 1226319-3.patch

Almost everything.

Need to file follow ups for:
* extra white space at bottom of tree component -- what's up with that?
* Focus/keyboard controls
* Tests (bug 1176056)
* Only display optimizations when a node has optimizations (nit)

Comment 12

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

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

Comment 13

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

Comment on attachment 8711522 [details] [diff] [review]
1226319-3.patch

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

Notes for myself

::: devtools/client/performance/components/optimizations-item.js
@@ +68,5 @@
> +        propString = ` (.${propertyName})`;
> +      }
> +    }
> +
> +    if (!site.hasSuccessfulOutcome) {

This whole block needs to go, oops

@@ +83,5 @@
> +      onClick: () => onViewSourceInDebugger(frameData.url, site.data.line),
> +      frame: {
> +        source: frameData.url,
> +      line: site.data.line,
> +      column: site.data.column,

indentation

@@ +97,5 @@
> +  },
> +
> +  _renderAttempts({ item: attempts }) {
> +    return dom.span({ className: "optimization-attempts" },
> +      `Attempts (${attempts.length})`

L10N

@@ +103,5 @@
> +  },
> +
> +  _renderTypes({ item: types }) {
> +    return dom.span({ className: "optimization-types" },
> +      `Types (${types.length})`

L10N

::: devtools/client/performance/components/optimizations.js
@@ +14,5 @@
> +const onClickTooltipString = frame =>
> +  L10N.getFormatStr("viewsourceindebugger",`${frame.source}:${frame.line}:${frame.column}`);
> +const JIT_TITLE = L10N.getStr("jit.title");
> +// If TREE_ROW_HEIGHT changes, be sure to change `var(--jit-tree-row-height)`
> +// in `devtools/client/themes/performance.css`

jit-optimizations.css

::: devtools/client/performance/modules/logic/tree-model.js
@@ +80,5 @@
>        // Insert the inflated frame into the call tree at the current level.
>        let frameNode;
>  
> +      if (frameKey==="GC") {
> +        console.log(calls, inflatedFrame, isMetaCategory);

oops

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8711522 [details] [diff] [review]
1226319-3.patch

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

Looks good, but a few comments below.

Biggest issue is that the tree isn't filling the whole sidebar and so expanding nodes makes other things invisible. I think you need to height:100% some things. Attaching screenshots.

::: devtools/client/locales/en-US/jit-optimizations.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE These strings are used inside the Performance Tools

Within jit view within perf tools

::: devtools/client/performance/components/optimizations-item.js
@@ +21,5 @@
> +
> +  shouldComponentUpdate(nextProps, nextState) {
> +    return this.props.item != nextProps.item
> +      || this.props.depth != nextProps.depth
> +      || this.props.expanded != nextProps.expanded;

Should this consider this.props.type and/or this.props.frameData? Backing up a bit more: is rendering these trees very hot? I thought they were relatively small (hundreds of items in the upper end). If it isn't actually hot, it isn't worth trying to optimizae `shouldComponentUpdate` because it is very easy to miss prop changes that you should consider but forget to and end up with a weirdly broken UI that doesn't update when you expect it to.

@@ +43,5 @@
> +      case "types":        content = this._renderTypes(this.props); break;
> +      case "attempt":      content = this._renderAttempt(this.props); break;
> +      case "type":         content = this._renderType(this.props); break;
> +      case "observedtype": content = this._renderObservedType(this.props); break;
> +      default: throw new Error("Optimization node does not have a type.", type);

Should this be a DTU.assert() call?

@@ +46,5 @@
> +      case "observedtype": content = this._renderObservedType(this.props); break;
> +      default: throw new Error("Optimization node does not have a type.", type);
> +    };
> +
> +    return dom.div({ className: `optimization-tree-item optimization-tree-item-${type}`,

Nit: put "{" bracket on next line w/ indent or align arrow and content to it below so that all arguments are at the same indentation level

::: devtools/client/performance/components/optimizations.js
@@ +18,5 @@
> +// in `devtools/client/themes/performance.css`
> +const TREE_ROW_HEIGHT = 14;
> +
> +const Optimizations = module.exports = createClass({
> +  displayName: "optimizations",

Nit: "Optimizations" with a capital "O"

@@ +134,5 @@
> +          return site.data.attempts;
> +        } else if (isObservedType(node)) {
> +          return getIonTypeForObserved(node);
> +        }
> +        throw new Error("Could not find a parent for optimization data node.", node);

Should this be a DTU.assert() call?

::: devtools/client/performance/performance-controller.js
@@ +19,5 @@
>  });
>  
> +var React = require("devtools/client/shared/vendor/react");
> +var ReactDOM = require("devtools/client/shared/vendor/react-dom");
> +var OptimizationsView = React.createFactory(require("devtools/client/performance/components/optimizations"));

Nit: s/OptimizationsView/Optimizations/ to match the import path

::: devtools/client/shared/components/frame.js
@@ +25,5 @@
>      frame: PropTypes.shape({
>        functionDisplayName: PropTypes.string,
>        source: PropTypes.string.isRequired,
>        line: PropTypes.number.isRequired,
> +      column: PropTypes.number,

If this is no longer required, then we should have a test that we don't output "file.js:1337:undefined" or anything messed up like that. We shouldn't render the second colon either.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: all nodes collapsed, and all seems well with the cosmos

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: expanding -- where did the other call inline go? where did the getprop IC go?

Comment 17

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

Comment on attachment 8711522 [details] [diff] [review]
1226319-3.patch

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

::: devtools/client/shared/components/frame.js
@@ +25,5 @@
>      frame: PropTypes.shape({
>        functionDisplayName: PropTypes.string,
>        source: PropTypes.string.isRequired,
>        line: PropTypes.number.isRequired,
> +      column: PropTypes.number,

This whole thing needs tests https://bugzilla.mozilla.org/show_bug.cgi?id=1243022

Comment 18

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

Attachment: 1226319-3.patch

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8712247 [details] [diff] [review]
1226319-3.patch

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

Looks good, but I want to insist on the small frame test I mentioned in the last review -- I'm not asking for comprehensive tests for the frame component in this patch, just this one behavior because I suspect it is incorrect right now. Look at the tree tests: it is super easy to write small, quick tests for these components.

Also, I applied this patch and built and the perf tool stopped working, will attach screenshot of console errors.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: errors in console

Comment 21

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

Attachment: 1226319-3.patch

Fixed tests, added tests for frame component

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

Comment 22

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

w/r/t attachment 8711834 [details] -- the bottom of the tree either expands for a ton of white space, and sometimes just covers up the bottom half of the container -- any idea why it does that? It's just the core tree view as far as I can tell

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #22)
> w/r/t attachment 8711834 [details] -- the bottom of the tree either expands
> for a ton of white space, and sometimes just covers up the bottom half of
> the container -- any idea why it does that? It's just the core tree view as
> far as I can tell

Do you have `height: 100%` and `overflow-y: scroll` on the tree's container(s)? The tree component inserts whitespace to fill the bottom and make the scrollbar position correct. First, make sure all this is set up properly (you may need to reference the memory tool's CSS).

If all that seems correct, then try setting a breakpoint on when the tree sets the height of the bottom spacer (maybe conditionally on if the height is larger than expected) and debugging from there.

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8712453 [details] [diff] [review]
1226319-3.patch

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

Thanks very much for the tests, they look great! r=me

::: devtools/client/shared/components/test/mochitest/test_frame_01.html
@@ +33,5 @@
> +    }), window.document.body);
> +    yield forceRender(frame);
> +    checkFrameString(frame, "mahscripts.js", 55, 10);
> +
> +    // Check when there's no column

How about also when the column is 0?

Comment 25

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

Comment on attachment 8712453 [details] [diff] [review]
1226319-3.patch

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

::: devtools/client/shared/components/test/mochitest/test_frame_01.html
@@ +33,5 @@
> +    }), window.document.body);
> +    yield forceRender(frame);
> +    checkFrameString(frame, "mahscripts.js", 55, 10);
> +
> +    // Check when there's no column

Had this in mind, but good idea to test as well

Comment 26

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

Tree's really weird.
http://i.imgur.com/CU8IyYF.gif

Values in the console.log: padding height, traversal length, begin, end

Tried many different variations of flex/overflow/height, what's in the current patch is mostly from the memory tool.

Can either land this as is or file follow ups (req'd for a "release" IMO, these tree failures make it pretty uncool), either way, I've burned an enormous amount of time on styling this and can't do more right now

Comment 27

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

Victor, is this something you can wizard out w/r/t the styles?

Comment 28

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

Attachment: 1226319-3.patch

Updated with test where column === 0

Comment 29

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

Landing -- it's better than what we have now, will file follow ups to make this blogworthy

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9b3c9c05e11c

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b3c9c05e11c

Comment 32

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

Closing this, broke out remaining issues in bug 1243906