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

Avoid running hasMetaBoxes on each subscribe #15041

Merged
merged 2 commits into from
Apr 19, 2019

Conversation

youknowriad
Copy link
Contributor

While monitoring the typing performance, I noticed that in the duraction of the key press event (53ms), 2 or 3ms is spent on calling hasMetaboxes selector. In this PR, I'm managing to tweak the side effect a little bit to avoid calling this selector on each subscribe.

While if we're trying to be pure, we should call the selector on each subscribe, in this particular use-cases where this is a legacy API that is only initialized at rendering, I think it's fine to optimize this way.

@youknowriad youknowriad added [Type] Performance Related to performance efforts Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Apr 18, 2019
@youknowriad youknowriad self-assigned this Apr 18, 2019
@youknowriad youknowriad requested a review from aduth April 18, 2019 11:16
@aduth
Copy link
Member

aduth commented Apr 18, 2019

2 or 3ms is spent on calling hasMetaboxes selector.

Is there a sense of why it has a noted impact? Curious if there's something problematic of the underlying selector.

I'd probably agree if we can be certain it's initialized once, it would be fine to move it out of the subscribe. Would be good to have a reference to this initialization.

Co-Authored-By: youknowriad <benguella@gmail.com>
@youknowriad youknowriad merged commit 3f1964c into master Apr 19, 2019
daniloercoli added a commit that referenced this pull request Apr 19, 2019
…rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor

* 'master' of https://github.com/WordPress/gutenberg:
  Avoid running hasMetaBoxes on each subscribe (#15041)
  Avoid passing down isFirst and isLast props (#15043)
  Add "Roadmap" document with an overview of projects in consideration. (#14907)
  Testing: Update tests to use Escape press to activate block toolbar (#15063)
  Testing: Skip unreliable end-to-end tests (#15059)
@youknowriad youknowriad deleted the update/metabox-performance-tweak branch April 19, 2019 11:34
@ellatrix
Copy link
Member

Looks like this was merged with failing tests.

@youknowriad
Copy link
Contributor Author

yep sorry about that missed it, I included the suggestion above and thought was harmless cause it was just a comment :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants