-
Notifications
You must be signed in to change notification settings - Fork 407
Instrument operation durations #642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coffee-fueled self-review powers: activate
| import {autobind} from 'core-decorators'; | ||
|
|
||
| import EventWatcher from '../event-watcher'; | ||
| import Switchboard from '../switchboard'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names are hard ¯\(ツ)/¯
| } | ||
|
|
||
| async unstageHunk(hunk) { | ||
| this.props.switchboard.didBeginStageOperation({unstage: true, hunk: true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little conflicted about following this pattern. Specifically, there are two alternatives:
- Separate Switchboard methods for every operation. Aggregation of "related" operations is the subscribers' responsibility.
- Individual Switchboard methods can be called in multiple related situations. Distinguishing context from the event payloads is the subscribers' responsibility.
On one hand, the metaprogramming I've set up in Switchboard makes it easy to quickly define a large number of events, which makes (1) more tenable. On the other, I feel like that could get out of hand?
| } else { | ||
| this.setState({...nullFilePatchState}, resolve); | ||
| this.setState({...nullFilePatchState}, () => { | ||
| this.props.switchboard.didFinishRender('RootController.showFilePatchForPath'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we'll eventually want didFinishRender() calls in every setState(), forceUpdate(), or ReactDom.render() callback. Is there a better way to hook those without muddying up the codebase?
| this.savedState = {}; | ||
|
|
||
| this.switchboard = new Switchboard(); | ||
| this.setupYardstick(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped this into a separate method to make it easy to comment out if it causes any trouble. Might just be a relic of me developing the mechanism, though.
| @@ -0,0 +1,74 @@ | |||
| import {Emitter} from 'atom'; // FIXME import from event-kit instead | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self to deal with this once #640 lands 😄
| 'BeginStageOperation', | ||
| 'FinishStageOperation', | ||
| 'ChangePatch', | ||
| 'FinishRender', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metaprogramming jazz hands
| // eslint-disable-next-line no-console | ||
| console.log('%cbegin %c%s:begin', | ||
| 'font-weight: bold', | ||
| 'font-weight: normal; font-style: italic; color: blue', this.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I went mad with power when I realized that console.log lets you style text.
| assert.isFalse(hunkView0.prop('isSelected')); | ||
|
|
||
| const opPromise0 = eventWatcher.getStageOperationPromise(); | ||
| const opPromise0 = switchboard.getFinishStageOperationPromise(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving all of these "getXyzPromise()" methods up into the Switchboard should be helpful, I think.
|
Merging because I'm about to cause a shit ton of conflicts otherwise 😇 |
Even if we don't have time for a full histogram or boxplot view before release (#634), I'd like to have some way to collect operation durations. Let's use the
EventHandlerand some basicperformance.now()checks to get some kind of numbers into JSON files or something.