Summary: | [Chameleon] Code Review - WidgetManager.php | ||
---|---|---|---|
Product: | Chameleon | Reporter: | Paul Spencer <pspencer@dmsolutions.ca> |
Component: | Core | Assignee: | Paul Spencer <pspencer@dmsolutions.ca> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | chameleon-dev@lists.maptools.org |
Priority: | P3 | Keywords: | CodeReview |
Version: | 1.99 | ||
Target Milestone: | 2.0 RC 1 | ||
Hardware: | All | ||
OS: | All | ||
Whiteboard: |
WidgetManager.php version 1.25 2003-10-31 - expand class comment to include better explanation of what the widget manager actually does, include example, clarify that this is internal to Chameleon. - add comments to class variables - constructor needs comments - investigate if foreach is faster than for? If not, replace foreach with for. - change " to ' - remove pre-registration of widgets from constructor, only build widget directory array. - in LoadWidget, test to see if the widget is loaded and if not, test for widget in each directory, load it, and register that it is loaded. Use class_exists() - LoadWidget needs function comment - is PreloadWidgets required? - missing function description for CreateWidgetUsingTag, remove @param szContent - add brackets to else on line 153 - line 127 - remove if, not required - replace ExtractClassName with preg match? Make sure that type="..." has a space at the beginning. (Julien) - check eval to see if it is causing a time problem (in CreateWidgetUsingTag) - remove lines 155-159 (handled in Widget.php) -
class comment done class variable comments done constructor comments added removed need for loop entirely changed " for ' where practical, still require a regexp for finding widget type to remove last one removed pre-registration of widgets modified LoadWidget to use class_exists and include in much tighter loop added function comment to LoadWidget removed PreloadWidgets added function comment to CreateWidgetUsingTag, removed szContext bracketed 'else' eval is not a performance hit (AFAI can tell) removed visibility code outstanding is the preg_match function for getting the type name out. BTW, include does not seem much faster than include_once ... this module is still performance bound by the number/size of files it has to include rather than any other factor. For a reasonably complete template with 50-60 widgets, this takes about 115 ms.
> eval is not a performance hit (AFAI can tell)
Ok, but I had made test (5 line script) before I got your comment. Here's the
result.
Eval can takes something like 3 times the normal time to execute simple calls,
but since we only do 50-60 class declaration call we probably only loss 1 millisec.
On a loop of 50 element that declare a "ttt" class each time, it takes 2
millisec instead of 1 to do it with an eval() call.
I also found that instead of:
$szEval = "\$oWidget =& new $szClass();";
eval($szEval);
you can do:
$oWidget =& new $szClass;
It will declare a class with the name in $szClass
I don't know if it worth the change in the WigetManager.php file, but I feel
that it's good to know for everyone. Maybe Paul can make the change to see which
impact it has on his test when he profile it.
given the limited number of times it is run, I expect little gain, but this is all about small improvements adding up. I will make this change. I believe that it may be more useful in MapLab, which also uses this technique.
need to finish the changes in here for 1.99
updated version to 1.99
Changed Target Milestone to 1.99 RC
this is as done as I want to make it. The final performance enhancement is negligible and hopefully unnecessary if we build a php module (WidgetManager would be one of the things replaced).
forgot to mark fixed.