[wp-trac] [WordPress Trac] #56975: Replace the internal `WP_Theme_JSON_Resolver::theme_has_support()` with a public function

[WordPress Trac]

#56975: Replace the internal `WP_Theme_JSON_Resolver::theme_has_support()` with a
public function
-------------------------------------------------+-------------------------
 Reporter:  oandregal                            |       Owner:
                                                 |  hellofromTonya
     Type:  enhancement                          |      Status:  reopened
 Priority:  normal                               |   Milestone:  6.2
Component:  Themes                               |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  gutenberg-merge has-patch has-unit-  |     Focuses:
  tests has-testing-info                         |  performance
-------------------------------------------------+-------------------------

Comment (by dmsnell):

 > This saves 40+ calls to is_readable which saves 48ms per page request.
 This is worth doing.

 Is this 48ms of a profiler run? Or is that 48ms of normal production
 runtime? I'm curious if we were to flip the tests and instead of measuring
 ms we measured req/s using something like `httperf` or `siege`. 48ms for
 `stat` calls to determine if a file exists seems off, even if it's called
 170 times. I think these tools might give more reliable readings than
 checking TTFB a few times and averaging, since they will hit a page
 thousands of times.

 A cursory search noted that repeated calls to `stat` should return data
 directly from memory and shouldn't require disk I/O after the first call,
 so it's possible that the measured impact of this in an artificial
 environment (such as a profiler) will appear much worse than it is in
 reality (not to mention what @oandregal pointed out in that the profiler
 substantially inflates reported runtimes across the board).

 Some ideas for getting out of the circle:

 Has anyone complained about this code slowing down their sites? If we
 don't know for sure that performance is a problem, we can follow what
 @Otto42 suggested and ship this without a cache. **Wait until we know it's
 a problem to fix it.**

 Ship without a cache so we can better gather data. A follow-up patch to
 add caching to an existing working system is a smaller problem than
 shipping a cache in advance. **No caching comes without its own cost** and
 more often than not, the costs of caching without a clear understanding of
 the implications of the cache result in defects, which are generally
 harsher than performance hits. We can take a patch adding a cache and
 apply it to existing sites with all their plugins and data and see **in
 realistic settings** what the impact is. As with the Gutenberg CI
 workflows, many if not most additive "optimizations" (such as adding
 caches) that have been applied to speed things up (and which had positive
 local micro-benchmark wins) have ended up not helping when situated within
 a bigger and more complicated system, but they all introduce latent bugs
 and maintenance burden.

 The fact that we're all recognizing a wide array of places where improper
 cache invalidation could triggers visible breakage in sites speaks to me
 that there's a high risk in caching this function the way we're talking
 about. I think this current discussion started when this same subsystem
 blocked all `wp-admin` CSS; if we're talking about similar levels of
 consequences, or worse (by handing out the wrong CSS for rendered frontend
 pages) then I'd definitely prefer some performance overhead.

 I'd be curious to see how "170+ times per page" really pans out against
 the context of all the other existing calls in a typical WP page render to
 know if it's a major problem or a drop in the bucket. Once we're looking
 at sites with a page cache in place (which hopefully is basically any and
 all actual production sites) how much does this cache matter since most
 requests will be served without ever contacting PHP? (talking about non-
 auth public page renders handled by something else, like `nginx`).

 Just some thoughts. Thanks for your continued diligence on this, and
 desire to avoid needless overhead.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56975#comment:96>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform