Make WordPress Core

#58309 closed enhancement (fixed)

Save a few processing cycles by removing redundant is_object

Reported by: presskopp's profile Presskopp Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch commit
Focuses: performance Cc:

Description

In addition to #58290 we could possibly remove more instances of is_object, namely in:

wp-includes\rest-api\fields\class-wp-rest-meta-fields.php, ~L.477
if ( is_object( $value ) && ! ( $value instanceof JsonSerializable ) ) {

wp-includes\class-json.php, ~L.929
} elseif (is_object($data) && ($data instanceof services_json_error ||

wp-includes\media.php, ~L.3762
|| is_object( $image ) && $image instanceof GdImage

However, one could argue that, for the sake of clear readability, it would be better to leave the is_object in, to indicate that an object is expected there. 58290 would need to be reverted then.

Attachments (2)

58309.diff (1.3 KB) - added by Presskopp 10 months ago.
58309.2.diff (2.0 KB) - added by costdev 10 months ago.
Reverse conditions in is_gd_image() too.

Download all attachments as: .zip

Change History (11)

#1 @Presskopp
10 months ago

Oh, seems like we also do patches for pclzip. In this case, let's speak about

\includes\class-pclzip.php, ~L. 1174
if (is_object($p_archive) && $p_archive instanceof pclzip)

\includes\class-pclzip.php, ~L.1238
if (is_object($p_archive_to_add) && $p_archive_to_add instanceof pclzip)

#2 @costdev
10 months ago

Thanks for opening this ticket @Presskopp!

wp-includes/rest-api/fields/class-wp-rest-meta-fields.php, ~L.477
if ( is_object( $value ) && ! ( $value instanceof JsonSerializable ) ) {

Strangely, I couldn't find this line in that file or in any file in Core. Can you double-check this one?

That said, it doesn't seem safe to change. It should only pass if $value is an object but not an instance of JsonSerializable. Removing is_object( $value ) would allow strings, ints, floats, arrays, etc. to pass. Though I haven't seen the source code in full yet.

wp-includes/class-json.php, ~L.929
} elseif (is_object($data) && ($data instanceof services_json_error ||

This file has been deprecated since 5.3.0 as the PHP native JSON extension is now a required extension.

wp-includes/media.php, ~L.3762
|| is_object( $image ) && $image instanceof GdImage

wp-admin/includes/class-pclzip.php, ~L. 1174
if (is_object($p_archive) && $p_archive instanceof pclzip)

wp-admin/includes/class-pclzip.php, ~L.1238
if (is_object($p_archive_to_add) && $p_archive_to_add instanceof pclzip)

These three seem fine to change.

Last edited 10 months ago by costdev (previous) (diff)

@Presskopp
10 months ago

#3 @Presskopp
10 months ago

  • Keywords has-patch added

Uh, I found the first one in a build folder somewhere. Let's just disregard it.

#4 @costdev
10 months ago

Thanks for the patch @Presskopp!

While we're here, we could make $image instanceof GdImage the first condition. So there'll only be one unnecessary check for a resource of 'gd', rather than two unnecessary checks for an object of GdImage.

What do you think?

#5 @Presskopp
10 months ago

I think we even can go for

if ( 'gd' === get_resource_type( $image ) || $image instanceof GdImage )

because the function get_resource_type first checks if the given parameter is a resource and only if so, returns it's type, else 'unknown' (if I get this correctly):

see zend_builtin_functions.c

Last edited 10 months ago by Presskopp (previous) (diff)

#6 @costdev
10 months ago

Unfortunately, get_resource_type() will return an error for a non-resource, so we need the is_resource()

This function will return null and generate an error if resource is not a resource.

#7 @Presskopp
10 months ago

Well then let's go with switching conditions, but I'm sure you know better then me when it comes to core patches.

@costdev
10 months ago

Reverse conditions in is_gd_image() too.

#8 @costdev
10 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.3

I've attached 58309.2.diff which makes $image instanceof GdImage the first condition, and updates the order of the checks mentioned in the docblock for good measure.

  • PHPUnit tests continue to pass.
  • Milestoning for 6.3.
  • Adding for commit consideration.

Committers: If this patch is ready for commit, we might consider turning is_gd_image() into a one-liner. Not a big deal though.

<?php

function is_gd_image( $image ) {
    return $image instanceof GdImage || is_resource( $image ) && 'gd' === get_resource_type( $image );
}

#9 @SergeyBiryukov
10 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 55757:

General: Remove a few is_object() checks followed by instanceof operator.

This is a minor performance enhancement:

  • If an object is passed, the call to is_object() will be redundant.
  • If a non-object is passed, the instanceof operator (a variant of is_a()) will first check if it is an object before doing any further processing.

Therefore, no additional processing cycles should be wasted in both cases.

Follow-up to [6779], [48798], [48905], [49194], [55748].

Props Presskopp, costdev.
Fixes #58309.

Note: See TracTickets for help on using tickets.