1. This site uses cookies. By continuing to use this site, you are agreeing to our use of cookies. Learn More.

Not Planned Compile Each Event Listener Type Into Single PHP File

Discussion in 'Closed Suggestions' started by digitalpoint, Oct 29, 2011.

  1. digitalpoint

    digitalpoint Well-Known Member

    I've been working more or less non-stop building all the vB customizations I've done over the years for XF.

    Most are just small little things, but in the end even small things take a bunch of PHP files when doing it MVC style. For example giving forums a default sort order requires 9 PHP files, 4 of which are listeners... so at a minimum it's adding 4 unique PHP being called for every page view and depending on how many models/template hooks are triggered, each of those are being called numerous times.

    By the time I'm done I'm probably going to have hundreds of Class Controller listeners alone... every page view is probably going to be including at least 500 unique PHP files, with a bunch of those being called multiple times.

    Maybe not a bad idea down the road to just grab all the enabled listeners and consolidate them into a single PHP file per listener type?
     
    Alfa1, BamaStangGuy, simbolo and 9 others like this.
  2. Fuhrmann

    Fuhrmann Well-Known Member

    Well, that's what i do in my add-ons...I create one single file for all listeners. Is that what you are asking? If it yes, works fine for me.

    PHP:
    /**
        * Extend some class
        *@since 1.0.0
        */
        
    public static function extend($class, array &$extend)
        {
            switch (
    $class) {
                case 
    'XenForo_ControllerPublic_Member':
                    
    $extend[] = 'BookmarkPosts_Extend_ControllerPublic_Member';
                    break;
                case 
    'XenForo_ControllerPublic_Post':
                    
    $extend[] = 'BookmarkPosts_Extend_ControllerPublic_Post';
                    break;
                case 
    'XenForo_ControllerPublic_Thread':
                    
    $extend[] = 'BookmarkPosts_Extend_ControllerPublic_Thread';
                    break;
                case 
    'XenForo_ControllerPublic_Account':
                    
    $extend[] = 'BookmarkPosts_Extend_ControllerPublic_Account';
                    break;
                case 
    'XenForo_Model_Post':
                    
    $extend[] = 'BookmarkPosts_Extend_Model_Post';
                    break;
                case 
    'XenForo_DataWriter_User':
                    
    $extend[] = 'BookmarkPosts_Extend_DataWriter_User';
                    break;
            }
        }
     
  3. digitalpoint

    digitalpoint Well-Known Member

    I've been consolidating them by controller type, but haven't been consolidating all types into a single one. I suppose that would probably be okay since there's usually at least one of every type loaded on every page. But I'm more thinking about what happens when I have 100 or so add-ons built, each with even just one controller listener.

    And for the record, I'm not adding a zillion things just to have them like arcades and all that. Most of them are backend/internal things... like allowing forum nodes to have a default sort order, having GeSHi-based CODE/PHP/HTML BBCode, post cache system to store pre-parsed posts, etc, etc...

    Just kind of worried what it's going to be like when it's including hundreds of extra listeners that are individually small.
     
  4. Floris

    Floris Guest

    class blah
    {
    function 1
    function 2
    thingy 3
    something 4
    etc
    }

    listener set to class name

    all in 1 file.

    using this to cache template, inject my own, update who's online, etc.
     
  5. EasyTarget

    EasyTarget Well-Known Member

    Hmmm 100's of listeners? Something doesn't seem right about this.

    I think if you know are going to be adding on significant amount of addons, you really need to think out the framework first. I would bet that you are repeating your actions numerous times and your listeners could be factorized better.

    The number of files you have in a php application really doesn't matter for performance. I would be concerned about the Heap memory requirements getting out of control. I would set up a PHP profiler i.e. http://xdebug.org/docs/profiler and determine if there is any craziness going on.
     
  6. digitalpoint

    digitalpoint Well-Known Member

    Well, most individual add-ons are going to require at least one listener. I've been tossing around the idea of just making all my custom stuff as one giant addon... but it would make maintenance of the code a little cumbersome as well as impossible to release anything individually since an "addon" is then really just a part of a giant conglomerate of custom addons I use.

    To put in in perspective, I have around 500 custom plug-ins I use for our vBulletin setup... on the surface one might wonder what in the hell all that stuff is doing since we don't have a ton of custom/extra stuff... see: http://forums.digitalpoint.com/

    But like I said earlier, the bulk of it are internal things to make the site faster. Some of those things I don't need to rewrite for XF, but then there are things I *do* need to write for XF that I didn't for vB, so it will probably be a wash. For example:

    I wrote a system that stores pre-parsed posts for XF (built into vB), which ends up being about a 3.5x page generation speed increase for XF (needs 3 different types of listeners to work): http://xenforo.com/community/threads/storage-of-parsed-posts.21904/#post-276766

    I wrote a system for XF that allows us to specify a default sort order on a per forum node basis (something we have come to rely on for our marketplace since we need stuff sorted by creation time)... this one needs 4 different types of listeners to work.

    I built a "Best Answer" system for XF to replace the one we use for vB (requires 1 listener).

    I built a replacement for our system that asks the user if a sub-forum might be a better place when they start a thread in an area with existing sub-forums (requires 2 listeners).

    We built BBCode so [thread] and [post] BBCode works in XF as well as replace CODE/PHP/HTML BBCode with a GeSHi based highlighter (2 listeners needed for those).

    You start adding up the little tweaks we have built for vB, and now rebuilding for XF and we are going to end up with a hundreds of listeners.

    The reality is that I have months of work ahead of me for building all the custom stuff we have for XF... so the 23 listeners I have in place now is sadly just the beginning. I have so much work ahead of me it's depressing just thinking about it. lol
     
    ge66 likes this.
  7. Mike

    Mike XenForo Developer Staff Member

    The simplest suggestion I would make is to only make one listener file per add-on - just call different methods in the same class. With APC, you'll be cutting stat calls but not necessarily wasting memory since it's already loaded.

    Not a ton you can do with the class extensions without folding add-ons together so that multiple things are done in one pass.
     
  8. digitalpoint

    digitalpoint Well-Known Member

    Even with 1 listener per addon, I'll probably have somewhere between 200-300 listeners when I'm done (did I mention I have a TON of work ahead of me? Oh yeah, I did... heh)

    I might take a look at the code that fires the listeners this weekend and see how realistic it's going to be to just patch the XF source to support a single compiled listener file for each type.

    WAY down the road... like maybe XenForo 2.0 or something, it might not be a bad idea if you could supply an optional (to not break existing stuff) hint when creating listeners... Like allow developers to call out the exact class a listener should be triggered for, rather than running all of them for all classes. If they supply no hint, then just run them globally like they are done now.
     
  9. Ramses

    Ramses Member

    In the end you're planning to move your forum to xenforo Shawn?
     
  10. Floris

    Floris Guest

    Do what ragtek did, make a framework.

    That way each individual add-on can hook into this framework, re-use already existing functions, etc.

    So at least repetitive code, phrases, etc can be called.

    If you have 500 customer pages.
    rather than 500x public blah blah within unique classes for listeners,
    you could just have a Online/Listener.php that handles them via an array or what not.
     
    James likes this.
  11. James

    James Well-Known Member

    My thoughts too. A framework for all of your add-ons wouldn't be a bad idea.
     
  12. Mike

    Mike XenForo Developer Staff Member

    That won't really resolve his issue though. It's not an issue of repeated code.
     
  13. Floris

    Floris Guest

    I know Mike, but surely if he has a basic set of files that the rest can use, he doesn't have to constantly make new classes and functions for them, and either re-use the ones available, or extend the array with the keys, or the case switches. Plus, the file is already cached if possible. Rather then 400 additional ones.
     
    James likes this.
  14. digitalpoint

    digitalpoint Well-Known Member

    I am using shared code whenever possible...

    Re: the actual topic (heh), it doesn't look like it's going to be terribly hard to patch the fire() method for code events to run a single one instead of looping, so not terribly worried about it.
     
  15. digitalpoint

    digitalpoint Well-Known Member

    So I've been giving this some more thought...

    While replacing the foreach() loop in XenForo_CodeEvent::fire() with a single compiled PHP file for each type would be an improvement for when you have a bunch of listeners, I'm thinking it might be better if you could optionally define a specific class (or template hook) for the listener when you create the event listener.

    Making it optional would make it so if the developer specified no class, then it's fired globally (just like it is now), and maintains backward compatibility (and would allow developers to kick off more advanced logic if needed just like it works now).

    Because the reality is that creating pseudo classes and running the logic for every one, for every XenForo_CodeEvent::fire() is a bit silly. It seems pretty standard that a page view triggers the fire() method 80-90 times per page (template hooks probably being most of them). Now let's say 50 of those are template hooks, and a site "only" had 10 template hook listeners for various template hooks... We are calling 500 static methods to really just determine if the template hook is being run is the one we want.

    Now if we could specify the template hook name as part of the listener... and call only the needed listeners (plus any global ones that didn't have a specific one specified), I think we would end up with a listener system that scales far better.

    That being said, the hook/listener system XF uses is fantastic... I absolutely love it. I just think it could benefit from not needing to call hundreds of static methods to determine IF we want to run the "real code". Getting it down to just running the listeners we actually need for the page being rendered would be a win.

    The way it works now, it would be like if your PHP app included every PHP file it had access when it initializes rather than just the ones it needs.

    Update: I ran a counter, and I show 29 unique PHP files (listeners) being included with all of them being called a total 484 times from the fire() event on the main page of my dev install for what ultimately ends up being the extension of 2 classes. If we could specify the class the listener is for, it would be SO much more efficient.
     
    HWS, Adam Howard, George and 3 others like this.
  16. digitalpoint

    digitalpoint Well-Known Member

    So I prototyped this out, and it seems to work really well... making all pages load significantly faster as well as allow scaling properly when you have a lot of customizations installed and at the same time maintains backward compatibility so nothing breaks.

    My main forum page loaded about 3x faster and went from firing a little more than 500 static methods (used to see if we want to fire the real code), down to 3 (only calling the ones that were actually needed).

    The hint works for any listener that has the first argument as a string... which are all the most commonly used events... The ones for loading classes and template hooks.

    If an addon developer specifies the OPTIONAL hint when defining their listener, that event will only fire when that template hook is actually called (in the case of a template_hook event), rather that firing every template hook listener for every possible template hook.

    If a developer does not specify a hint, it will just run globally like it does now.

    I added 1 column to the xf_code_event_listener table called "hint", changed the XenForo_Model_CodeEvent::getEventListenerArray() method to this (adding the hint stuff):
    PHP:
    ]    public function getEventListenerArray()
        {
            
    $output = array();

            
    $listenerResult $this->_getDb()->query('
                SELECT listener.event_id, listener.callback_class, listener.callback_method, listener.hint
                FROM xf_code_event_listener AS listener
                LEFT JOIN xf_addon AS addon ON
                    (addon.addon_id = listener.addon_id AND addon.active = 1)
                WHERE listener.active = 1
                    AND (addon.addon_id IS NOT NULL OR listener.addon_id = \'\')
                ORDER BY listener.event_id ASC, listener.execute_order
            '
    );
            while (
    $listener $listenerResult->fetch())
            {
                if (
    strlen($listener['hint'] == 0)) $listener['hint'] = '_';
                
    $output[$listener['event_id']][$listener['hint']][] = array($listener['callback_class'], $listener['callback_method']);
            }

            return 
    $output;
        }
    I changed the Code_Event::fire() method to this:
    PHP:
        public static function fire($event, array $args = array())
        {
            if (!
    self::$_listeners)
            {
                return 
    true;
            }

            if (!empty(
    self::$_listeners[$event]['_']))
            {
                foreach (
    self::$_listeners[$event]['_'] AS $callback)
                {
                    
    //TODO: Need some friendly error handling around this
                    
    if (is_callable($callback))
                    {            
                        
    $return call_user_func_array($callback$args);
                        if (
    $return === false)
                        {
                            return 
    false;
                        }
                    }
                }
            }

            if (@
    is_string($args[0]))
            {
                if (!empty(
    self::$_listeners[$event][$args[0]]))
                {
                    foreach (
    self::$_listeners[$event][$args[0]] AS $callback)
                    {
                        
    //TODO: Need some friendly error handling around this
                        
    if (is_callable($callback))
                        {            
                            
    $return call_user_func_array($callback$args);
                            if (
    $return === false)
                            {
                                return 
    false;
                            }
                        }
                    }
                }
            }

            return 
    true;
        }
    I didn't build the UI stuff for it since I really just needed to see if it would work and if it actually sped things up (it does). So the hint field would need to be added to the listener setup form as well and it would need to be exported/imported for addons.

    Hopefully we could see something along these lines natively in XF... if not it's going to be "to the diff/patch" system for my install. :)
     
    shawn, simbolo, Walter and 13 others like this.
  17. Trekkan

    Trekkan Well-Known Member

    I'm not an addon dev, but... I can appreciate and understand what you just did there... I sure hope Mike and Kier are looking at this, could be really valuable to add into the core.
     
  18. digitalpoint

    digitalpoint Well-Known Member

    As a side benefit of having an optional "hint" for event listeners, you could more or less consolidate all the similar event listeners into a single one. All the load_class* event listeners could just be a generic "load_class" listener with a drop-down of the supported classes. It would make adding new listeners for a class a matter of adding it to the drop-down list.

    I really hope we see something similar to the "hint" system I posted above... My dev setup currently runs 1,100 static methods for a single page view (1,100 checks to see IF something should be run), when in the end it's running 3. The static methods to check if something should be extended end up compounding exponentially (and it's the event types that could have hints that are the worst). If you have a total of 10 listeners for load_class_model and you have 5 different models that load on that page, you end up making 50 calls to static methods to see if anything should load... similar to template_hook... you could have 20 template_hook listeners, and a single page could easily have 40 template hooks (especially if there's a hook/template within a foreach loop)... you just had to make 800 static method calls for what ultimately might be 1 actual hook you want to run.
     
    EQnoble, Adam Howard, HWS and 2 others like this.
  19. Adam Howard

    Adam Howard Well-Known Member

    I'd like to see this put into action for XenForo (please)
     
    HWS likes this.
  20. Marcus

    Marcus Well-Known Member

    I added the hint row and replaced the existing with the both changed functions. This deactivated the addon system for me. If I use the build in fire() function, my addons work.

    Could you describe as an example how to work with your hint system and your adPositioning? That would be great!
     

Share This Page