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.

Sign up for GitHub

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

Jetpack Sync: Checksum performance optimizations for Meta Sync Module #41390

Merged
merged 8 commits into from
Jan 30, 2025
+260 −26
Merged

Jetpack Sync: Checksum performance optimizations for Meta Sync Module #41390

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d29e0f5
Jetpack Sync: Checksum performance optimizations for Meta Sync Module
fgiannar Jan 29, 2025
78a3250
changelog
fgiannar Jan 29, 2025
e7d0ec3
Hotfix for edge case where the max length is reached in the last loop
fgiannar Jan 29, 2025
856ec38
Make phan happy
fgiannar Jan 29, 2025
fb69c92
Update phan baseline gile for sync package
fgiannar Jan 29, 2025
92e223e
Introduce fetch_prepared_meta_from_db
fgiannar Jan 30, 2025
c54754b
Remove class-meta.php from exclude list - no lint errors
fgiannar Jan 30, 2025
9d7799e
Sync package: Update phan baseline file
fgiannar Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion projects/packages/sync/.phan/baseline.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
'src/modules/class-full-sync-immediately.php' => ['PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeMismatchReturn'],
'src/modules/class-full-sync.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeComparisonFromArray'],
'src/modules/class-import.php' => ['PhanTypeMismatchArgumentInternal'],
'src/modules/class-meta.php' => ['PhanParamSignatureMismatch', 'PhanTypeArraySuspicious', 'PhanTypeMismatchArgumentProbablyReal'],
'src/modules/class-meta.php' => ['PhanParamSignatureMismatch', 'PhanTypeArraySuspicious'],
'src/modules/class-module.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchReturnProbablyReal'],
'src/modules/class-network-options.php' => ['PhanParamSignatureMismatch', 'PhanTypeMismatchReturnProbablyReal'],
'src/modules/class-options.php' => ['PhanParamSignatureMismatch', 'PhanTypeMismatchReturnProbablyReal'],
Expand Down
4 changes: 4 additions & 0 deletions projects/packages/sync/changelog/update-sync-meta-module
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Jetpack Sync: Checksum performance optimizations for Meta Sync Module
97 changes: 83 additions & 14 deletions projects/packages/sync/src/modules/class-meta.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ public function name() {
* @return array
*/
public function get_objects_by_id( $object_type, $config ) {
global $wpdb;

$table = _get_meta_table( $object_type );

if ( ! $table ) {
Expand All @@ -48,13 +50,66 @@ public function get_objects_by_id( $object_type, $config ) {
return array();
}

$meta_objects = array();
$object_id_column = $object_type . '_id';
$object_key_pairs = array();

foreach ( $config as $item ) {
$meta = null;
if ( isset( $item['id'] ) && isset( $item['meta_key'] ) ) {
$meta = $this->get_object_by_id( $object_type, (int) $item['id'], (string) $item['meta_key'] );
$object_key_pairs[ (int) $item['id'] ][] = (string) $item['meta_key'];
}
}

$conditionals = array();
$where_sql = '';
$current_query_length = 0;

foreach ( $object_key_pairs as $object_id => $keys ) {
fgiannar marked this conversation as resolved.
Show resolved Hide resolved
$keys_placeholders = implode( ',', array_fill( 0, count( $keys ), '%s' ) );
$where_condition = trim(
$wpdb->prepare(
// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared
"( `$object_id_column` = %d AND meta_key IN ( $keys_placeholders ) )",
array_merge( array( $object_id ), $keys )
)
);

$where_sql = empty( $where_sql ) ? $where_condition : $where_sql . ' OR ' . $where_condition;

$current_query_length += strlen( $where_sql );

if ( $current_query_length > self::MAX_DB_QUERY_LENGTH ) {
$conditionals[] = $where_sql;
$where_sql = '';
$current_query_length = 0;
}
}

if ( ! empty( $where_sql ) ) {
$conditionals[] = $where_sql;
}

$meta_objects = array();

foreach ( $conditionals as $where ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract the code inside the foreach to a method and call it inside the ifs of lines 80 and 87?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion! Done with 92e223e but I refactored a lot of things so we'll need to re-test everything now to be on the safe side.

$meta = $wpdb->get_results( // phpcs:ignore WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery
// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared
"SELECT * FROM {$table} WHERE {$where}",
ARRAY_A
);

if ( ! is_wp_error( $meta ) && ! empty( $meta ) ) {
foreach ( $meta as $meta_entry ) {
$object_id = $meta_entry[ $object_id_column ];
$meta_key = $meta_entry['meta_key'];
$key = $object_id . '-' . $meta_key;

if ( ! isset( $meta_objects[ $key ] ) ) {
$meta_objects[ $key ] = array();
}

$meta_objects[ $key ][] = $this->get_prepared_meta_object( $object_type, $meta_entry );
}
}
$meta_objects[ $item['id'] . '-' . $item['meta_key'] ] = $meta;
}

return $meta_objects;
Expand Down Expand Up @@ -94,19 +149,33 @@ public function get_object_by_id( $object_type, $id = null, $meta_key = null ) {

if ( ! is_wp_error( $meta ) && ! empty( $meta ) ) {
foreach ( $meta as $meta_entry ) {
if ( 'post' === $object_type && strlen( $meta_entry['meta_value'] ) >= Posts::MAX_POST_META_LENGTH ) {
$meta_entry['meta_value'] = '';
}
$meta_objects[] = array(
'meta_type' => $object_type,
'meta_id' => $meta_entry['meta_id'],
'meta_key' => $meta_key,
'meta_value' => $meta_entry['meta_value'],
'object_id' => $meta_entry[ $object_id_column ],
);
$meta_objects[] = $this->get_prepared_meta_object( $object_type, $meta_entry );
}
}

return $meta_objects;
}

/**
* Accepts a DB meta entry and returns it in a standard format.
*
* @param string $object_type The meta object type, eg 'post', 'user' etc.
* @param array $meta_entry A meta array.
* @return array
*/
private function get_prepared_meta_object( $object_type, $meta_entry ) {
$object_id_column = $object_type . '_id';

if ( 'post' === $object_type && strlen( $meta_entry['meta_value'] ) >= Posts::MAX_POST_META_LENGTH ) {
$meta_entry['meta_value'] = '';
}

return array(
'meta_type' => $object_type,
'meta_id' => $meta_entry['meta_id'],
'meta_key' => $meta_entry['meta_key'],
'meta_value' => $meta_entry['meta_value'],
'object_id' => $meta_entry[ $object_id_column ],
);
}
}
9 changes: 9 additions & 0 deletions projects/packages/sync/src/modules/class-module.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ abstract class Module {
*/
const ARRAY_CHUNK_SIZE = 10;

/**
* Max query length for DB queries.
*
* @access public
*
* @var int
*/
const MAX_DB_QUERY_LENGTH = 15 * 1024;

/**
* Sync module name.
*
Expand Down
5 changes: 5 additions & 0 deletions projects/plugins/jetpack/changelog/update-sync-meta-module
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: other
Comment: Updated Sync related unit tests


103 changes: 102 additions & 1 deletion projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-meta.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

class WP_Test_Jetpack_Sync_Meta extends WP_Test_Jetpack_Sync_Base {
protected $post_id;
protected $meta_id;
protected $meta_module;

protected $whitelisted_post_meta = 'foobar';
Expand All @@ -25,7 +26,7 @@ public function set_up() {
$this->meta_module = Modules::get_module( 'meta' );
Settings::update_settings( array( 'post_meta_whitelist' => array( 'foobar' ) ) );
$this->post_id = self::factory()->post->create();
add_post_meta( $this->post_id, $this->whitelisted_post_meta, 'foo' );
$this->meta_id = add_post_meta( $this->post_id, $this->whitelisted_post_meta, 'foo' );
$this->sender->do_sync();
}

Expand Down Expand Up @@ -321,4 +322,104 @@ public function test_get_object_by_id_size_limit_max() {
$metas = $module->get_object_by_id( 'post', $this->post_id, $this->whitelisted_post_meta );
$this->assertEquals( $meta_test_value, $metas[0]['meta_value'] );
}

/**
* Tests get_objects_by_id
*/
public function test_get_objects_by_id() {
$module = Modules::get_module( 'meta' );
$meta_id = add_post_meta( $this->post_id, $this->whitelisted_post_meta, 'bar' );
$config = array(
array(
'id' => $this->post_id,
'meta_key' => $this->whitelisted_post_meta,
),
);
$metas = $module->get_objects_by_id( 'post', $config );

$expected = array(
$this->post_id . '-' . $this->whitelisted_post_meta => array(
array(
'meta_type' => 'post',
'meta_id' => (string) $this->meta_id,
'meta_key' => $this->whitelisted_post_meta,
'meta_value' => 'foo',
'object_id' => (string) $this->post_id,
),
array(
'meta_type' => 'post',
'meta_id' => (string) $meta_id,
'meta_key' => $this->whitelisted_post_meta,
'meta_value' => 'bar',
'object_id' => (string) $this->post_id,
),
),
);
$this->assertSame( $expected, $metas );
}

/**
* Verify that meta_values above size limit are truncated in get_objects_by_id.
*/
public function test_get_objects_by_id_size_limit_exceeded() {
$meta_test_value = str_repeat( 'X', Posts::MAX_POST_META_LENGTH );
update_post_meta( $this->post_id, $this->whitelisted_post_meta, $meta_test_value );

$module = Modules::get_module( 'meta' );
$config = array(
array(
'id' => $this->post_id,
'meta_key' => $this->whitelisted_post_meta,
),
);
$metas = $module->get_objects_by_id( 'post', $config );

$expected = array(
$this->post_id . '-' . $this->whitelisted_post_meta => array(
array(
'meta_type' => 'post',
'meta_id' => (string) $this->meta_id,
'meta_key' => $this->whitelisted_post_meta,
'meta_value' => '',
'object_id' => (string) $this->post_id,
),
),
);
$this->assertSame( $expected, $metas );
}

/**
* Tests get_objects_by_id when the max DB query length is exceeded.
*/
public function test_get_objects_by_id_max_query_length_exceeded() {
$module = Modules::get_module( 'meta' );
$long_meta_key = str_repeat( 'X', $module::MAX_DB_QUERY_LENGTH );
// We are not actually adding the meta, only in $config so that it produces a long DB query.
$config = array(
array(
'id' => 9999,
'meta_key' => $long_meta_key,
),
array(
'id' => $this->post_id,
'meta_key' => $this->whitelisted_post_meta,
),
);

$metas = $module->get_objects_by_id( 'post', $config );

$expected = array(
$this->post_id . '-' . $this->whitelisted_post_meta => array(
array(
'meta_type' => 'post',
'meta_id' => (string) $this->meta_id,
'meta_key' => $this->whitelisted_post_meta,
'meta_value' => 'foo',
'object_id' => (string) $this->post_id,
),
),
);

$this->assertSame( $expected, $metas );
}
}
Loading