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

High memory usage in various /report pages #816

Closed
pkevan opened this issue Jan 16, 2023 · 8 comments · Fixed by #826
Closed

High memory usage in various /report pages #816

pkevan opened this issue Jan 16, 2023 · 8 comments · Fixed by #826

Comments

@pkevan
Copy link
Contributor

pkevan commented Jan 16, 2023

Describe the bug

Alerts are consistently occurring when visiting various report pages e.g. https://central.wordcamp.org/reports/payment-activity-report/

To reproduce

Steps to reproduce the behavior:

  1. Go to https://central.wordcamp.org/reports/payment-activity-report/
  2. Observe memory usage through a plugin such as query monitor

Expected behavior

Less memory used :)

@pkevan
Copy link
Contributor Author

pkevan commented Jan 16, 2023

From what I have observed, the high memory usage appears to be down to the limitless query to grab all previous WordCamps for the dropdown, and associated meta: https://github.com/WordPress/wordcamp.org/blob/production/public_html/wp-content/mu-plugins/4-helpers-wcpt.php#L19-L35

@pkevan
Copy link
Contributor Author

pkevan commented Jan 16, 2023

Given it's an optional field, perhaps we should either cache the resultant data or reduce the number selected.

Currently we return 150k meta fields and 1900 WordCamps.

@iandunn
Copy link
Member

iandunn commented Jan 17, 2023

Yeah, get_wordcamp_dropdown() only needs Start Date (YYYY-mm-dd), so maybe we couldt add a meta_keys argument to get_wordcamp()?

The arg could be false to include no meta, true to include all meta (the default), or an array of meta keys to only fetch those specific ones.

Would caching reduce the memory usage, or just make the queries faster? I would assume that any extra/duplicate data from the cache-miss process would be garbage collected, but could be wrong.

@pkevan
Copy link
Contributor Author

pkevan commented Jan 18, 2023

Would caching reduce the memory usage, or just make the queries faster?

The memory issue is due to all the data from the limitless WordCamp query. By reducing the amount of results the amount of memory on the page and time taken to render is reduced (time not the primary problem here).

Removing the meta doesn't appear to fix the memory used in testing.

pkevan added a commit that referenced this issue Jan 19, 2023
Fixes #816

By reducing the amount of results by default (100), and narrowing results based on year input, the memory usage on the page is dramatically reduced.
@iandunn
Copy link
Member

iandunn commented Jan 19, 2023

The memory issue is due to all the data from the limitless WordCamp query. By reducing the amount of results the amount of memory on the page and time taken to render is reduced (time not the primary problem here).

Yeah, I understand that part, but I was confused when you mentioned caching. I was assuming you meant doing something like:

function get_wordcamps( $args = array() ) {
	$args = wp_parse_args(
		$args,
		array(
			'post_type'   => WCPT_POST_TYPE_ID,
			'post_status' => 'any',
			'orderby'     => 'ID',
			'numberposts' => -1,
			'perm'        => 'readable',
		)
	);

+	$cache_key   = __FUNCTION__ . '::' . md5( wp_json_encode( $args ) );
+	$cached_data = get_transient( $cache_key );
+	
+	if ( $cached_data ) {
+		return $cached_data;
+	}

Since it's a similar amount of data, it doesn't seem like it'd affect memory usage at first glance. Were you thinking of something different?

@iandunn
Copy link
Member

iandunn commented Jan 20, 2023

I just remembered that the reports are already using transient caching via maybe_get_cached_data(). If you were referring to static page caching, we use WP Super Cache for that (related #771). But these reports are also accessed in wp-admin, so those wouldn't get static page caching.

@iandunn
Copy link
Member

iandunn commented Jan 20, 2023

Removing the meta doesn't appear to fix the memory used in testing

🤔 that's surprising, if we reduce the returned array to just something like...

array(
	array(
		'id'.        => 1024,
		'title'      => 'WordCamp Narnia 2022',
		'start-date' => '2022-04-15',
	),

	// ...
)

... then it should only be about 50k bytes for 1300 sites in my crude calculation. I'd assume that the rest of the fetched data would be garbage collected.

We could also optimize the query so that it only fetches what's needed, rather than fetching everything and reducing it in PHP.

@pkevan pkevan mentioned this issue Jan 20, 2023
@pkevan
Copy link
Contributor Author

pkevan commented Jan 20, 2023

We could also optimize the query so that it only fetches what's needed, rather than fetching everything and reducing it in PHP.

That's where I got to in testing some caching of the data - the data returned is vastly more than is needed (full post object)

pkevan added a commit that referenced this issue Jan 25, 2023
Reduce data gathered to fields used in dropdown through direct query.

Fixes #816
@pkevan pkevan mentioned this issue Jan 25, 2023
pkevan added a commit that referenced this issue Feb 22, 2023
* Fix/816 3

Reduce data gathered to fields used in dropdown through direct query.

Fixes #816

* Remove post_status field

* Remove whitespace.
iandunn added a commit that referenced this issue Feb 24, 2023
Otherwise the query won't work in local environments where the site IDs don't match production.

See #816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants