Ticket #3155 (closed enhancement: fixed)

Opened 2 years ago

Last modified 5 months ago

Several "notice" messages in WP 2.1-alpha3

Reported by: quix0r Assigned to: Nazgul
Priority: high Milestone: 2.5
Component: General Version: 2.1
Severity: major Keywords: needs-patch early has-patch-partial
Cc:

Description

As you may know notice messages from the Apache is slowing down every PHP driven software because the interpreter needs some time to generate those notices. Even surpressing them does not turn off the investigation of the intepreter in e.g. a missing variable, array index and so on.

I have fixes now all notices on the admin dashboard, index (the default post with default comment) and with no plugins installed and enabled.

Attachments

hotfix-20060921-1928.diff (32.4 kB) - added by quix0r on 09/21/06 17:41:37.
hotfix-20060921-2009.diff (42.4 kB) - added by quix0r on 09/21/06 18:34:58.
sql cacher, element cacher and notice-fixes
3155adddebug.diff (0.5 kB) - added by Nazgul on 09/21/07 07:01:40.
3155-part1.diff (10.9 kB) - added by Nazgul on 01/31/08 22:37:21.
First batch

Change History

09/21/06 17:41:37 changed by quix0r

  • attachment hotfix-20060921-1928.diff added.

09/21/06 18:03:03 changed by masquerade

Just a little sidenote fyi, notices will not slow down execution, and have nothing to do with Apache. The PHP interpreter does the checks whether or not a notice needs to be generated, otherwise it wouldn't know if a notice needed to be generated, so MFHing them for the purpose of speeding up the code is the wrong reason. That doesn't mean they don't need to be fixed, but I wouldn't get the idea that fixing them is optimizing anything. That's one of those things that falls under "premature optimization".

09/21/06 18:31:34 changed by quix0r

Okay, I reset the settings. Btw: new attachment added. Now the following functions are "cached" by the element cacher I have written:

- wptexturize - wpautop - sanitize_user - sanitize_title_with_dashes - url_to_postid - mysql2date - get_option

I did this because my profiler showed me they where using too much time. My cacher will compare the given settings in parameter list with it's internal hash list. The hash will be created by the parameters + recommended and optional WP_SECRET. If the hash was found it returns the value from the cache otherwise it got added after the main part of the function.

If you don't understand this please take wptexturize() as an example and look in my element-cache.php script for details. I have added lots of one-line comments to my scripts so you will maybe understand it a little. If you don't mind I can also attach here both profiler reports. :-)

The added SQL cacher (wp-db_cache.php) is experimental and unfinished.

09/21/06 18:33:08 changed by quix0r

  • priority changed from high to normal.
  • component changed from Optimization to General.

Opps, got booted after typing text... :-(

09/21/06 18:34:58 changed by quix0r

  • attachment hotfix-20060921-2009.diff added.

sql cacher, element cacher and notice-fixes

09/21/06 18:59:07 changed by quix0r

Please try this text script out to prove it yourself:

<?php //error_reporting(E_ALL);

// Init $result = array(); $index = 1;

$start = explode(" ", microtime()); $start = $start[0] + $start[1]; for ($idx = 0; $idx < 1000000; $idx++) {

$result[$index+$idx] = $idx;

}

$end = explode(" ", microtime()); $end = $end[0] + $end[1];

echo "Time: ".($end - $start);

?>

First don't comment the line behind // Init out. Execute the script and wait... Write down the time and comment it out. Run the script again. Now compare both times... :-)

Attention: If you comment in error_reporting this will produce a very big "page" in your browser.

09/21/06 20:09:38 changed by ringmaster

There is a lot of prior work dealing with WordPress and caches, and some decisions had been made to offload some of those demands onto server software that is more dedicated to the task.

For instance, qcache is going to do a much better job at handling caching than interpreted PHP. Using APC will enhance your execution performance to the point that leaving wptexturize() et. al. uncached will produce insignificant impact.

There is a built-in object cache in WP that is disabled by default because it seems that only certain sites benefit from the enhancements under particular circumstances. In some cases the cache actually slows down execution when it's not badly needed.

It might be handy to remove the PHP errors that appear in E_ALL mode, but are these changes really critical? I would also take care that you're not harming intended functionality.

For example, there's a slew of:

if(!isset($qp?)) $qp? = ;

...to integrate into the query engine. Some of the later code may require that the value is unset. By setting it, you may affect code farther down in the execution path.

Making grand changes to working code for the sake of removing notices (not errors or even warnings) from the code in the most extreme reporting setting is unwise without significant code review to make sure things are still working as expected.

Looking at your sample, I notice that you're using the value of an unset variable inside the loop. That may delay the loop execution, but I doubt you'll see any section of WordPress code that repeatedly accesses an unset variable in this way. Perhaps if there was an obvious instance of this and you focused your patch on addressing that individual concern, it would be easier to recommend for commit. As is, this patch is too broad.

09/21/06 20:54:52 changed by ryan

We already have a cache. Why not use that one? As ringmaster says, the object cache is pluggable and can use memcached or what-have-you on the backend. Even with the default persistent cache turned off, the object cache still does in-memory, per-load caching.

09/21/06 22:11:04 changed by quix0r

I leave the text below the line unchanged because I have written it before I have read the email... :-/

So it is true that some PHP versions take longer because of malloc to create an empty variable than "handling" the undefined variable? I guess this is a "flaw" in PHP itself and the discussion shall not be held here. So I don't continue at this point. :-)

Maybe the element cacher is useless or is a negative impact on performance (time for execusion). But what about my idea of caching SQL results? Please read the next below anyway. I have described it there more detailed. :-)

The attached pprofp.log is a profile report of a vanilla 2.1-alpha3.


Hmmm... In my Intranet with no plugins installed it makes no difference to have the element cache enabled or disabled. In both cases I have an execution time around 0.20 secs. But I'm still not giving up to convince you. :-) ;-)

I need some more plugins installed to test it more detailed.

And the element cache is written for reducing function calls by caching the results of the functions (I will try to cache more than the above listing). It is definedly not a time-safer, "only" a call-safer cache.

My first target with this cache where the functions which are always and everywhere on the blog used regardless if you post a comment, write a post, or just view the blog.

My next target are time-consuming queries (have you seen my profiler-result?) by caching their results in local files. Surely the cache must be purged when the cached tables (which where extracted from the query string) got updated by INSERT, UPDATE or DELETE (ALTER and CREATE, of course too but this will not happen so often... ;-) ).

The results of my profiler (I used Advanced PHP Debugger's "pprofp") did show all one thing: mysql_fetch_object() and other functions where the "heaviest" functions PHP has to execute.

So my idea here was to cache the result to reduce usage of these heavy functions.

You may want to check-out my blog for more informations who I want to handle this. :-) There you also find the original profiler output. Strange: After I fixed the "notice messages" the number of fucntion calls was recudes significantly. ... Hmmm.... :-?

09/21/06 22:57:41 changed by foolswisdom

  • severity changed from major to normal.
  • milestone changed from 2.1 to 2.2.

02/13/07 20:21:57 changed by tombarta

Is this a ticket about E_NOTICE errors or about caching? These are orthogonal issues and it's confusing to see discussion on both of them in a single ticket. IMHO, Wordpress should not be producing E_NOTICE anywhere. The pattern that typically fixes this without affecting large swaths of code is converting

if ( $my_super_option != '' ) ...

into

if ( isset($my_super_option) && ($my_super_option != '') ) ...

For readability's sake, it's best to avoid conditional defining of variables, so checks like if ($option) make sense without referencing undefined vars.

03/27/07 22:38:36 changed by foolswisdom

  • milestone changed from 2.2 to 2.3.

07/25/07 17:18:22 changed by Nazgul

  • keywords set to needs-patch.

Also reported in #4672, #4673, #4675 and #4676.

07/25/07 23:12:19 changed by foolswisdom

  • milestone changed from 2.3 (trunk) to 2.4 (future).

09/20/07 15:15:24 changed by Nazgul

  • keywords changed from needs-patch to needs-patch early.
  • priority changed from normal to high.
  • severity changed from normal to major.

I suggest fixing those notice messages early in the 2.4 cycle.

Because the impact is quite big, we need the time to iron out the wrinkles.

09/21/07 06:55:30 changed by Nazgul

09/21/07 07:01:40 changed by Nazgul

  • attachment 3155adddebug.diff added.

09/21/07 07:04:20 changed by Nazgul

Added a patch which makes it possible to enable debugging in wp-config for (plugin) developers.

Based on code by Ozh from a post on wp-hackers.

09/21/07 20:23:26 changed by Nazgul

See also #5033

09/26/07 22:33:07 changed by Nazgul

  • owner changed from anonymous to Nazgul.
  • status changed from new to assigned.

The attached patches by quix0r are stale.

I've turned notice messages on and will post partial patches here as I come across them.

09/26/07 23:05:53 changed by Nazgul

  • keywords changed from needs-patch early to needs-patch early has-patch-partial.

Partial patch which fixes almost all notice warnings on my blog frontpage attached.

09/30/07 11:11:00 changed by westi

A change like 3155adddebug.diff has gone into trunk in [6179] for #5033

10/01/07 16:35:38 changed by ryan

We can use empty() in some of those places to avoid ands. Also, doing the empty check first will avoid needing the issets in query.php.

01/31/08 22:37:21 changed by Nazgul

  • attachment 3155-part1.diff added.

First batch

01/31/08 22:38:08 changed by Nazgul

Refreshed my (partial) patch based on ryan's suggestions.

02/02/08 18:42:09 changed by ryan

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [6711]) Some notice fixes from Nazgul. fixes #3155