Skip to content

Conversation

@jstejada
Copy link
Contributor

@jstejada jstejada commented Aug 26, 2021

Summary

When rebasing my profiling PR (#22167) I encountered a lot of merge conflicts, so I figured it might make sense to provide similar utils for performance marks, both to make it easier to manage my merge conflicts, but also a lot of the branching introduced into the code.

Just a proposal, so let me know if you think this makes sense, but also totally fine to abandon this refactor.

Test Plan

  • yarn flow
  • yarn test
  • yarn test-build-devtools
  • manual test of browser extension

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I'm generally a fan of this change. Seems like the resulting code is more readable.

The one nit I find myself thinking about is the naming of these functions. The naming pattern of "withAsyncPerformanceMark" feels slightly odd to me. The purpose of these functions is to measure code, so maybe something like "measureAsyncCode" / "measureSyncCode" would read more naturally?

This is a super nit though. Feel free to ignore if you disagree.

Copy link
Contributor

@lunaruan lunaruan left a comment

Choose a reason for hiding this comment

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

Yay refactoring for code quality! 😃

@jstejada
Copy link
Contributor Author

bikeshedding: for context, the reason i used the with* naming is kind of to indicate that i'm "wrapping" it in something, in this case performance marks. using a name that's a verb like measureSyncCode or something makes me think that we're returning a measurement, or something of that sort, but also not a big deal

@jstejada jstejada merged commit 0ba0564 into facebook:main Aug 26, 2021
@jstejada jstejada deleted the performance-mark-utils branch August 30, 2021 21:17
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
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.

4 participants