Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add highlight render option to Render Performance tab #1685

Merged
merged 13 commits into from
Oct 18, 2021

Conversation

michaelbdai
Copy link
Contributor

@michaelbdai michaelbdai commented Sep 3, 2021

Description

for issue #1684
Added a checkbox in the render performance tab/route
When enabled, the component gets highlighted when rends.
See more details in screen recording.

Screenshots

Here is the screen recording and unpacked build
https://drive.google.com/file/d/1jIUPaM69okID1tlroAFu8lead6zeKfi_/view?usp=sharing

@nummi
Copy link
Collaborator

nummi commented Sep 16, 2021

It's beautiful!

ember_debug/models/profile-manager.js Outdated Show resolved Hide resolved
ember_debug/models/profile-manager.js Show resolved Hide resolved
ember_debug/models/profile-manager.js Outdated Show resolved Hide resolved
ember_debug/models/profile-manager.js Outdated Show resolved Hide resolved
ember_debug/models/profile-manager.js Outdated Show resolved Hide resolved
ember_debug/models/profile-manager.js Outdated Show resolved Hide resolved
@michaelbdai michaelbdai marked this pull request as draft October 1, 2021 07:24
@lifeart
Copy link
Contributor

lifeart commented Oct 11, 2021

this is performance profile node constructor https://github.com/emberjs/ember-inspector/blob/master/ember_debug/models/profile-node.js#L17

looks like here payload for performance profile node is generated https://github.com/emberjs/ember.js/blob/master/packages/@ember/instrumentation/index.ts

and this is glimmer component manager https://github.com/glimmerjs/glimmer-vm/blob/master/packages/%40glimmer/manager/lib/public/component.ts and yes, looks like it don't have such instrumentation builtin

as part of this mr emberjs/ember.js#19293 glimmer component manager does not belongs to ember itself

and this is emberjs/ember.js#11099 intent to move instrumentation to component managers

@chancancode @rwjblue do you have ideas about way to add rerender instrumentation for glimmer components?

@MelSumner
Copy link

@rwwagner90 please review :)

@RobbieTheWagner
Copy link
Member

@MelSumner we're not merging new things until the build is fixed and working with beta and canary, but we will definitely review all open PRs when that time comes.

@RobbieTheWagner
Copy link
Member

@michaelbdai now that master has passing tests, can you please rebase?

@michaelbdai
Copy link
Contributor Author

michaelbdai commented Oct 17, 2021

@rwwagner90 rebased. all tests passing now.

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this super in depth PR @michaelbdai! I especially appreciate that you added so many great tests! I just had a few minor questions, but otherwise, this looks good to me.

app/controllers/render-tree.js Outdated Show resolved Hide resolved
app/controllers/render-tree.js Show resolved Hide resolved
app/routes/render-tree.js Show resolved Hide resolved
app/controllers/render-tree.js Show resolved Hide resolved
@RobbieTheWagner RobbieTheWagner merged commit f7e2143 into emberjs:master Oct 18, 2021
@RobbieTheWagner
Copy link
Member

Thanks again so much for the PR! 🎉

patricklx pushed a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
* add highlight render option to Render Performance tab

clean up

* fix lint error and change per comment

* Use bounds to find component instead of id

* change per comment

* change per comments

* change per comments

* change per comments

* added test

* replace timeout with later

* use async

* improve tests

* add test for glimmer component, to show the highlight is not supported.

* change per comments

Co-authored-by: Bing Dai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants