WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#3155 closed enhancement (fixed)

Several "notice" messages in WP 2.1-alpha3

Reported by: quix0r Owned by: Nazgul
Milestone: 2.5 Priority: high
Severity: major Version: 2.1
Component: General Keywords: needs-patch early has-patch-partial
Focuses: 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 (4)

hotfix-20060921-1928.diff (32.4 KB) - added by quix0r 8 years ago.
hotfix-20060921-2009.diff (42.4 KB) - added by quix0r 8 years ago.
sql cacher, element cacher and notice-fixes
3155adddebug.diff (526 bytes) - added by Nazgul 7 years ago.
3155-part1.diff (10.9 KB) - added by Nazgul 6 years ago.
First batch

Download all attachments as: .zip

Change History (26)

quix0r8 years ago

comment:1 masquerade8 years ago

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".

comment:2 quix0r8 years ago

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.

comment:3 quix0r8 years ago

  • Component changed from Optimization to General
  • Priority changed from high to normal

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

quix0r8 years ago

sql cacher, element cacher and notice-fixes

comment:4 quix0r8 years ago

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.

comment:5 ringmaster8 years ago

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.

comment:6 ryan8 years ago

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.

comment:7 quix0r8 years ago

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.... :-?

comment:8 foolswisdom8 years ago

  • Milestone changed from 2.1 to 2.2
  • Severity changed from major to normal

comment:9 tombarta7 years ago

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.

comment:10 foolswisdom7 years ago

  • Milestone changed from 2.2 to 2.3

comment:11 Nazgul7 years ago

  • Keywords needs-patch added

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

comment:12 foolswisdom7 years ago

  • Milestone changed from 2.3 (trunk) to 2.4 (future)

comment:13 Nazgul7 years ago

  • Keywords early added
  • 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.

Nazgul7 years ago

comment:15 Nazgul7 years ago

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.

comment:16 Nazgul7 years ago

See also #5033

comment:17 Nazgul7 years ago

  • 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.

comment:18 Nazgul7 years ago

  • Keywords has-patch-partial added

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

comment:19 westi7 years ago

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

comment:20 ryan7 years ago

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.

Nazgul6 years ago

First batch

comment:21 Nazgul6 years ago

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

comment:22 ryan6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.