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 User Timings for the Interactivity API #60522

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Apr 5, 2024

What?

Add performance marks to DevTools to assist with performance analysis.

Note: Debug code should be placed behind a build flag so that it can be excluded from production builds.

Why?

  • Adding timing metrics to the Interactivity API will enable identifying costly or long running callbacks triggered by the API.
  • Leverages the User Timing API to assist with debugging Interactivity API code.

Screenshots

image
Screenshot showing various Interactivity API event timings - note that I added a random delay in the callback time to make the bars more visible here.

Testing Instructions

Note: testing currently requires Chrome Canary. To enable the User Timings API, in Dev Tools Settings -> Experiments - enable Performance User Timings.

  1. Create a page with blocks using the Interactivity API ("IA")
  2. Open the DevTools performance panel and start recording.
  3. Interact with the IA items and stop recording
  4. Check the performance timeline for the timing metrics for each callback
  5. Longer callbacks will result in longer bars

@gziolo gziolo added the [Packages] Interactivity /packages/interactivity label Apr 6, 2024
@adamsilverstein adamsilverstein added the [Type] Performance Related to performance efforts label Apr 10, 2024
@adamsilverstein adamsilverstein changed the title WIP add some user timings for interactivity API WIP add User Timings for the Interactivity API Apr 16, 2024
@adamsilverstein
Copy link
Member Author

This PR is ready for testing, I am leaving it in the draft state for now until the DevTools extension API lands (without having to set a flag).

@adamsilverstein adamsilverstein marked this pull request as ready for review May 28, 2024 17:57
Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@adamsilverstein adamsilverstein changed the title WIP add User Timings for the Interactivity API Add User Timings for the Interactivity API Jul 2, 2024
@adamsilverstein
Copy link
Member Author

Is anything needed code wise here to indicate this is an experimental feature? Should I add inline comments indicating that?

@gziolo
Copy link
Member

gziolo commented Jul 11, 2024

Is anything needed code wise here to indicate this is an experimental feature? Should I add inline comments indicating that?

if ( globalThis.SCRIPT_DEBUG ) { check removes it from production so that is one way to guard it. If you want, you can also make it only available in the Gutenberg plugin by using a similar check with globalThis.IS_GUTENBERG_PLUGIN. The only important aspect is to play with the production mode to ensure that the code is properly removed when combining conditions. I don't remember how sophisticated that process is with analyzing more complex conditions.

@adamsilverstein
Copy link
Member Author

Is anything needed code wise here to indicate this is an experimental feature? Should I add inline comments indicating that?

if ( globalThis.SCRIPT_DEBUG ) { check removes it from production so that is one way to guard it. If you want, you can also make it only available in the Gutenberg plugin by using a similar check with globalThis.IS_GUTENBERG_PLUGIN. The only important aspect is to play with the production mode to ensure that the code is properly removed when combining conditions. I don't remember how sophisticated that process is with analyzing more complex conditions.

OK, thanks for the tips. I will test out the IS_GUTENBERG_PLUGIN constant. I think for now/initially it would be fine to keep only in the plugin. I assume that means it is not included in code that gets merged to core? I feel we can ask developers building on the Interactivity API to install the plugin in order to debug performance metrics.

@gziolo
Copy link
Member

gziolo commented Jul 15, 2024

I assume that means it is not included in code that gets merged to core?

Yes, it never executes in core when using IS_GUTENBERG_PLUGIN. It will get fully removed from code on production. SCRIPT_DEBUG ensures the code runs in development but not on production.

@gziolo gziolo added the Developer Experience Ideas about improving block and theme developer experience label Jul 24, 2024
@adamsilverstein
Copy link
Member Author

@gziolo when I tried adding both conditionals:

if ( globalThis.SCRIPT_DEBUG && globalThis.IS_GUTENBERG_PLUGIN ) {

I got linting errors:

error  `globalThis.SCRIPT_DEBUG` should only be used as the condition in an if statement or ternary expression         @wordpress/wp-global-usage
error  `globalThis.IS_GUTENBERG_PLUGIN` should only be used as the condition in an if statement or ternary expression  @wordpress/wp-global-usage
etc...

So in 9e37958 I wrapped the SCRIPT_DEBUG in the IS_GUTENBERG_PLUGIN conditional. We can probably remove the inner conditional once this API becomes stable.

@gziolo
Copy link
Member

gziolo commented Jul 30, 2024

So in 9e37958 I wrapped the SCRIPT_DEBUG in the IS_GUTENBERG_PLUGIN conditional. We can probably remove the inner conditional once this API becomes stable.

That’s totally fine 👍

@adamsilverstein adamsilverstein merged commit 144f27c into WordPress:trunk Jul 30, 2024
59 checks passed
@adamsilverstein adamsilverstein deleted the add/interactivity-api-user-timings branch July 30, 2024 15:28
@github-actions github-actions bot added this to the Gutenberg 19.0 milestone Jul 30, 2024
@fabiankaegy fabiankaegy added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Oct 1, 2024
@fabiankaegy fabiankaegy mentioned this pull request Oct 1, 2024
97 tasks
@DAreRodz
Copy link
Contributor

@adamsilverstein, this PR's been marked as "Needs Dev Note". Could you help write one? 🙏

cc: @fabiankaegy

@fabiankaegy
Copy link
Member

@DAreRodz Looks like this actually is Gutenberg only and not in 6.7. So we don't need one after all

@fabiankaegy fabiankaegy removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Packages] Interactivity /packages/interactivity [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants