Connection\Manager: is_connected: Memoize #39361

mreishus · 2024-09-11T18:39:16Z

Proposed changes:

Memoize the is_connected() function, so it's only figured out once per request
- Also a short boolean optimization, for a && b, if a is false, we don't need to figure out b.

Reasoning

I noticed it popping up a lot while I reviewed timelines for my local site running JP. I used ac_start() and ac_stop() to measure its timing (link) before and after adding memoization.

Before

[             ] https://myJTsite.example.com/wp-cron.php?doing_wp_cron=1726077077.1958370208740234375000
[             a19432] AC Debug: 'is_connected1' ran 38 times, total time: 5.82 ms, avg time: 0.15 ms
[             ] https://myJTsite.example.com/
[             c43038] AC Debug: 'is_connected1' ran 32 times, total time: 4.33 ms, avg time: 0.14 ms
[             ] https://myJTsite.example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[             adcb58] AC Debug: 'is_connected1' ran 45 times, total time: 3.83 ms, avg time: 0.09 ms
[             ] https://myJTsite.example.com/
[             d41c05] AC Debug: 'is_connected1' ran 31 times, total time: 3.8 ms, avg time: 0.12 ms
[             ] https://myJTsite.example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[             1b0db2] AC Debug: 'is_connected1' ran 45 times, total time: 5.83 ms, avg time: 0.13 ms
[             ] https://myJTsite.example.com/

after

[             f07557] AC Debug: 'is_connected2' ran 31 times, total time: 0.17 ms, avg time: 0.01 ms
[             ] https://myJTsite.example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[             baec98] AC Debug: 'is_connected2' ran 45 times, total time: 0.19 ms, avg time: 0 ms
[             ] https://myJTsite.example.com/
[             cdd611] AC Debug: 'is_connected2' ran 31 times, total time: 0.15 ms, avg time: 0 ms
[             ] https://myJTsite.example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[             6dcbd5] AC Debug: 'is_connected2' ran 45 times, total time: 0.14 ms, avg time: 0 ms
[             ] https://myJTsite.example.com/
[             38bb3d] AC Debug: 'is_connected2' ran 31 times, total time: 0.11 ms, avg time: 0 ms
[             ] https://myJTsite.example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[             299baf] AC Debug: 'is_connected2' ran 45 times, total time: 0.13 ms, avg time: 0 ms
[             ] https://myJTsite.example.com/wp-cron.php?doing_wp_cron=1726077292.4192719459533691406250
[             1f2e26] AC Debug: 'is_connected2' ran 36 times, total time: 0.16 ms, avg time: 0 ms
[             ] https://myJTsite.example.com/
[             50359e] AC Debug: 'is_connected2' ran 32 times, total time: 0.21 ms, avg time: 0.01 ms
[             ] https://myJTsite.example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[             863e0f] AC Debug: 'is_connected2' ran 45 times, total time: 0.18 ms, avg time: 0 ms

Concerns

Would there be any need to invalidate this? For example, a request that starts out connected and then is later not connected? Or vice versa? I don't know the JP codebase well enough to know if this is something that would be accounted for. If so, it's not a big deal, I just don't know where that would be or how to test it.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Unknown :( General connection working? Connect / Disconnect?

github-actions · 2024-09-11T18:40:03Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/is-connected branch.
- For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack update/is-connected

bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/is-connected

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2024-09-11T18:40:12Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

oskosk · 2024-09-11T18:59:10Z

looking

oskosk · 2024-09-11T19:32:26Z

@sergeymitr is also looking at this. He mentioned the checks are failing cause we're not resetting the state between tests /

I tested launching a site from scratch (JN), disconnecting, and reconnecting and it looked fine.

sergeymitr · 2024-09-11T19:48:49Z

I believe we should also reset self::$is_connected on site registration and disconnect when we know for sure that the value has changed.

mreishus · 2024-09-12T00:02:37Z

Ok, got the tests to pass at least. I want to consider these things tomorrow:

Is there still a benefit after the invalidation added
Possibly adding the hook(s) I need to the jetpack options class instead of invalidating on every write to the jetpack private options
Seeing if there's a better way to deal with WorDBless

mreishus · 2024-09-12T15:43:27Z

Is there still a benefit after the invalidation added?

tldr: yes

There is still a benefit. The invalidation does not really run as you are doing casual browsing of the site. However, I refreshed my site with the latest WP and added an object cache client using local memcache. The penalty of running is_connected decreased, but it still looks like a 1-2ms gain.

Some requests without the change:

[home         d9f7da] https://example.com/
[home         d9f7da] AC Debug: 'is_connected, trunk' ran 31 times, total time: 1.66 ms, avg time: 0.05 ms
[admin        c33968] https://example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[admin        c33968] AC Debug: 'is_connected, trunk' ran 44 times, total time: 1.92 ms, avg time: 0.04 ms
[home         d4a438] https://example.com/
[home         d4a438] AC Debug: 'is_connected, trunk' ran 31 times, total time: 1.41 ms, avg time: 0.05 ms
[admin        5f6d64] https://example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[admin        5f6d64] AC Debug: 'is_connected, trunk' ran 44 times, total time: 1.97 ms, avg time: 0.04 ms
[home         e14818] https://example.com/
[home         e14818] AC Debug: 'is_connected, trunk' ran 31 times, total time: 1.38 ms, avg time: 0.04 ms
[admin        4b7393] https://example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[admin        4b7393] AC Debug: 'is_connected, trunk' ran 44 times, total time: 1.88 ms, avg time: 0.04 ms
[wp-admin     e7f34e] https://example.com/wp-admin/
[wp-admin     e7f34e] AC Debug: 'is_connected, trunk' ran 48 times, total time: 2.38 ms, avg time: 0.05 ms
[visits       e817a3] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/visits?unit=day&quantity=7&date=2024-09-12&stat_fields=views%2Cvisitors
[visits       e817a3] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.49 ms, avg time: 0.05 ms
[top-posts    adbdfa] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/top-posts?period=day&num=7&date=2024-09-12&summarize=1&max=0
[referrers    e9a70d] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/referrers?period=day&num=7&date=2024-09-12&summarize=1&max=0
[data         7b687d] https://example.com/wp-json/jetpack/v4/module/protect/data
[data         c4b424] https://example.com/wp-json/jetpack/v4/module/akismet/data
[referrers    e9a70d] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.33 ms, avg time: 0.05 ms
[data         7b687d] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.28 ms, avg time: 0.04 ms
[top-posts    adbdfa] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.4 ms, avg time: 0.05 ms
[data         c4b424] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.34 ms, avg time: 0.05 ms
[wp-admin     2fa0bf] https://example.com/wp-admin/
[wp-admin     2fa0bf] AC Debug: 'is_connected, trunk' ran 48 times, total time: 2.08 ms, avg time: 0.04 ms
[visits       09be2c] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/visits?unit=day&quantity=7&date=2024-09-12&stat_fields=views%2Cvisitors
[visits       09be2c] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.33 ms, avg time: 0.05 ms
[top-posts    f22531] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/top-posts?period=day&num=7&date=2024-09-12&summarize=1&max=0
[referrers    03aa48] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/referrers?period=day&num=7&date=2024-09-12&summarize=1&max=0
[data         c90049] https://example.com/wp-json/jetpack/v4/module/protect/data
[data         3eb388] https://example.com/wp-json/jetpack/v4/module/akismet/data
[data         3eb388] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.33 ms, avg time: 0.05 ms
[top-posts    f22531] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.25 ms, avg time: 0.04 ms
[referrers    03aa48] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.25 ms, avg time: 0.04 ms
[data         c90049] AC Debug: 'is_connected, trunk' ran 29 times, total time: 1.33 ms, avg time: 0.05 ms

Some requests with the change:

[admin-ajax   e53484] https://example.com/wp-admin/admin-ajax.php
[home         abbbc2] https://example.com/
[wp-cron      4affa8] https://example.com/wp-cron.php?doing_wp_cron=1726153216.1312329769134521484375
[wp-cron      4affa8] AC Debug: 'is_connected, memoized' ran 35 times, total time: 0.08 ms, avg time: 0 ms
[home         abbbc2] AC Debug: 'is_connected, memoized' ran 31 times, total time: 0.07 ms, avg time: 0 ms
[admin        d9da63] https://example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[admin        d9da63] AC Debug: 'is_connected, memoized' ran 44 times, total time: 0.09 ms, avg time: 0 ms
[wp-admin     c6bfff] https://example.com/wp-admin/
[wp-admin     c6bfff] AC Debug: 'is_connected, memoized' ran 48 times, total time: 0.07 ms, avg time: 0 ms
[visits       3a333b] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/visits?unit=day&quantity=7&date=2024-09-12&stat_fields=views%2Cvisitors
[visits       3a333b] AC Debug: 'is_connected, memoized' ran 29 times, total time: 0.05 ms, avg time: 0 ms
[data         e9b8cc] https://example.com/wp-json/jetpack/v4/module/protect/data
[data         e38e0b] https://example.com/wp-json/jetpack/v4/module/akismet/data
[referrers    119daa] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/referrers?period=day&num=7&date=2024-09-12&summarize=1&max=0
[top-posts    2e22e7] https://example.com/wp-json/jetpack/v4/stats-app/sites/12345/stats/top-posts?period=day&num=7&date=2024-09-12&summarize=1&max=0
[data         e38e0b] AC Debug: 'is_connected, memoized' ran 29 times, total time: 0.04 ms, avg time: 0 ms
[data         e9b8cc] AC Debug: 'is_connected, memoized' ran 29 times, total time: 0.04 ms, avg time: 0 ms
[referrers    119daa] AC Debug: 'is_connected, memoized' ran 29 times, total time: 0.05 ms, avg time: 0 ms
[top-posts    2e22e7] AC Debug: 'is_connected, memoized' ran 29 times, total time: 0.08 ms, avg time: 0 ms
[home         990a7f] https://example.com/
[home         990a7f] AC Debug: 'is_connected, memoized' ran 31 times, total time: 0.11 ms, avg time: 0 ms
[admin        737a3c] https://example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[admin        737a3c] AC Debug: 'is_connected, memoized' ran 44 times, total time: 0.07 ms, avg time: 0 ms
[home         9e3152] https://example.com/
[home         9e3152] AC Debug: 'is_connected, memoized' ran 31 times, total time: 0.1 ms, avg time: 0 ms
[admin        38fe92] https://example.com/wp-admin/admin.php?page=stats&noheader&proxy&chart=admin-bar-hours-scale
[admin        38fe92] AC Debug: 'is_connected, memoized' ran 44 times, total time: 0.07 ms, avg time: 0 ms

Is there a better way to deal with WorDBless?

tldr: no

I don't see one, since their options functions are very simple and don't fire hooks.

Changing hooks in jetpack options class

Not needed. I was able to get everything passing by invalidating off the right pre_update_jetpack_option_ hooks.

mreishus · 2024-09-17T19:50:04Z

I'm seeing a benefit on logged out homepage loads on a WoA site as well:

[             ] AC Debug: 'connected' ran 36 times, total time: 4.63 ms, avg time: 0.13 ms
[             ] AC Debug: 'connected' ran 36 times, total time: 4.56 ms, avg time: 0.13 ms
[             ] AC Debug: 'connected' ran 36 times, total time: 4.64 ms, avg time: 0.13 ms
---
[             ] AC Debug: 'connected' ran 36 times, total time: 0.25 ms, avg time: 0.01 ms
[             ] AC Debug: 'connected' ran 36 times, total time: 0.25 ms, avg time: 0.01 ms
[             ] AC Debug: 'connected' ran 36 times, total time: 0.26 ms, avg time: 0.01 ms

I'd like to merge this soon if there are no objections @jeherve @oskosk

mreishus · 2024-09-19T19:12:26Z

I was previously using ::configure() to add the invalidation hooks, which ->is_connected() would rely on to make sure it didn't keep anything cached for too long. While it's convention to call ::configure() for each enabled module, I found that on simple sites, ->is_connected() is sometimes called without ::configure() being called.
Because it's only convention that ::configure() is always called first, and it's possible for ->is_connected() to be called without ::configure() to be called, I added an extra boolean flag to track if the invalidation has been added, and allowed ->is_connected() to add the invalidation itself if ::configure() was bypassed somehow. I couldn't find an actual situation where this mattered, but just wanted to be ultra-safe.

…options updates

…pack_private_options

…nfigure() is not called

mreishus self-assigned this Sep 11, 2024

mreishus added the [Status] Needs Team Review label Sep 11, 2024

github-actions bot added the [Package] Connection label Sep 11, 2024

mreishus requested review from jeherve and oskosk September 11, 2024 18:42

oskosk added the [Focus] Performance label Sep 11, 2024

github-actions bot added [Package] Protect Status [Package] Stats Data [Tests] Includes Tests labels Sep 11, 2024

jeherve requested a review from a team September 12, 2024 08:56

mreishus force-pushed the update/is-connected branch 2 times, most recently from e6dc371 to 6781af1 Compare September 12, 2024 14:21

mreishus force-pushed the update/is-connected branch from ad3c94d to 3ff0b60 Compare September 17, 2024 15:38

mreishus force-pushed the update/is-connected branch from 3ff0b60 to 43d61ad Compare September 19, 2024 17:53

mreishus added 7 commits September 23, 2024 09:17

Connection\Manager: is_connected: Memoize

f5ac282

changelog

0fa685b

add reset_connection_status and hook it on disconnection, registration

1834beb

add some invalidations

525e8d4

WorDBless invalidations

72328be

changelog

8a6910f

WorDBless fixes for packages/connection

9ded357

mreishus added 5 commits September 23, 2024 09:17

Remove these three actions, not needed if we get all jetpack_private_…

7939a84

…options updates

fix on multisite

7ee0fb7

Only invalidate on certain option writes instead of everything in jet…

12320ee

…pack_private_options

extract to add_connection_status_invalidation_hooks

b5fa851

allow the connection status invalidation hooks to be added even if co…

ca63743

…nfigure() is not called

mreishus force-pushed the update/is-connected branch from 6fc41ea to ca63743 Compare September 23, 2024 14:17

mreishus merged commit 202fec8 into trunk Sep 23, 2024
73 checks passed

mreishus deleted the update/is-connected branch September 23, 2024 14:46

github-actions bot removed the [Status] Needs Team Review label Sep 23, 2024

gogdzl pushed a commit that referenced this pull request Oct 25, 2024

Connection\Manager: is_connected: Memoize (#39361)

61b1d63

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection\Manager: is_connected: Memoize #39361

Connection\Manager: is_connected: Memoize #39361

mreishus commented Sep 11, 2024

github-actions bot commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024

oskosk commented Sep 11, 2024

oskosk commented Sep 11, 2024

sergeymitr commented Sep 11, 2024

mreishus commented Sep 12, 2024 •

edited

Loading

mreishus commented Sep 12, 2024

mreishus commented Sep 17, 2024

mreishus commented Sep 19, 2024

Connection\Manager: is_connected: Memoize #39361

Connection\Manager: is_connected: Memoize #39361

Conversation

mreishus commented Sep 11, 2024

Proposed changes:

Reasoning

Concerns

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Sep 11, 2024 • edited Loading

github-actions bot commented Sep 11, 2024

oskosk commented Sep 11, 2024

oskosk commented Sep 11, 2024

sergeymitr commented Sep 11, 2024

mreishus commented Sep 12, 2024 • edited Loading

mreishus commented Sep 12, 2024

Is there still a benefit after the invalidation added?

Is there a better way to deal with WorDBless?

Changing hooks in jetpack options class

mreishus commented Sep 17, 2024

mreishus commented Sep 19, 2024

github-actions bot commented Sep 11, 2024 •

edited

Loading

mreishus commented Sep 12, 2024 •

edited

Loading