[WIP] Significant reduction in memory consumption

[creocoder]

With this fix:

    [CComponent:_e] => array
    (
        'onbeforesave' => CList#8
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#9
                    (
                        [createAttribute] => 'create_time'
                        [updateAttribute] => 'update_time'
                        [setUpdateOnCreate] => false
                        [timestampExpression] => null
                        [CBehavior:_enabled] => true
                        [CBehavior:_owner] => Test#1(...)
                        [CComponent:_e] => null
                        [CComponent:_m] => null
                    )
                    1 => 'beforeSave'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
    )

Without:

    [CComponent:_e] => array
    (
        'onafterconstruct' => CList#16
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17
                    (
                        [createAttribute] => 'create_time'
                        [updateAttribute] => 'update_time'
                        [setUpdateOnCreate] => false
                        [timestampExpression] => null
                        [CBehavior:_enabled] => true
                        [CBehavior:_owner] => Test#1(...)
                        [CComponent:_e] => null
                        [CComponent:_m] => null
                    )
                    1 => 'afterConstruct'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onbeforevalidate' => CList#18
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'beforeValidate'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onaftervalidate' => CList#19
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'afterValidate'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onbeforesave' => CList#20
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'beforeSave'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onaftersave' => CList#21
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'afterSave'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onbeforedelete' => CList#22
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'beforeDelete'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onafterdelete' => CList#23
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'afterDelete'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onbeforefind' => CList#24
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'beforeFind'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
        'onafterfind' => CList#25
        (
            [CList:_d] => array
            (
                0 => array
                (
                    0 => CTimestampBehavior#17(...)
                    1 => 'afterFind'
                )
            )
            [CList:_c] => 1
            [CList:_r] => false
            [CComponent:_e] => null
            [CComponent:_m] => null
        )
    )

[creocoder]

@qiangxue What do you think about this catch? :)

[qiangxue]

It's a good catch, but the fix has the drawback that it makes the methods implicit and thus the corresponding API doc is gone.

Instead of using this fix, one can work around the issue by overriding the events() method so that only the needed event handlers are attached.

[creocoder]

@qiangxue Yes you right. But behavior developer need to think always about events() method. May be we can support him on core level? With API docs I think we can do something...

[creocoder]

@qiangxue Another approach here. Without drawbacks (i think).

[creocoder]

Seems my solution have side effects with behaviors child classes. I think we need add note about override event() in doc for behaviors delevopers and fix framework behaviors.

[samdark]

BC break? I don't think we want it. Still reducing memory consumption is a very good thing.

[slavcodev]

public function attach($owner)
{
    $this->_owner=$owner;
    if(($events=$this->events())!==array()){
        $reflector=new ReflectionClass($this);
        foreach($events as $event=>$handler){
            $methodClass=$reflector->getMethod($handler)->class;
            if($methodClass!=='CModelBehavior'&&$methodClass!=='CActiveRecordBehavior')
                $owner->attachEventHandler($event,array($this,$handler));
        }
    }
}

BC don't break and work with children classes.

[samdark]

@slavcodev It will not work for custom components extended from CComponent or CApplicationComponent. I don't think it's a good idea to hadcode class names.

[samdark]

I think we can go with blank methods removal. It's BC. Problem with IDEs completion can be solved via some additional PHPDoc. Documentation from methods can be moved to class header.

[qiangxue]

I think what @slavcodev suggested is a good compromise. The memory issue is important mainly when there are many instances of the same component class. This mainly happens to ActiveRecord.

I'm against removing the methods. Although we can put the needed doc into the class header, it's not neat. More importantly, we don't have class header inheritance (i.e. methods defined in the parent class won't have their doc showing in the child class header, if we choose to remove the methods.)

[samdark]

@qiangxue so you think fixing it for two specific classes is enough?

[creocoder]

I don't think it's a good idea to hadcode class names too. I vote for method_exists() + move doc to header.

Why:

• faster than without fix
• solve memory problems

[qiangxue]

@samdark It's enough for most cases. This problem mainly happens to base classes. It's not common that people will develop new base behavior classes. If they do, they can use the class_exists() approach to avoid the memory issue.

[creocoder]

I insist on the fact that the fix should be the same as in the last commit.

Without fix
Time: 1.50965 s
Memory: 12,467KB

With fix
Time: 0.55263 s
Memory: 5,681KB

Yii::beginProfile('test');
$array=array();
for($i=0;$i<10000;$i++)
    $array[]=new Test;
Yii::endProfile('test');

[qiangxue]

What about the performance result of what @slavcodev suggested? And how did you do the test?

[creocoder]

@qiangxue:

@slavcodev:

Time: 0.64722 s
Memory: 5,693KB

[qiangxue]

Then I vote for @slavcodev's approach.
For the removing method approach, consider this scenario: a developer wants to create a AR behavior and do something about beforeValidate(). He checks the API doc for CActiveRecordBehavior, but there's no a single place mentioning about beforeValidate() if we remove the declaration of beforeValidate(). He will only be able to find the info about beforeValidate() when he checks the parent class doc.

[creocoder]

@qiangxue I have more interesting idea. Will show shortly.

[creocoder]

@qiangxue I think we have ideal solution in last commit:

Time: 0.80 s
Memory: 5,722KB

There is no hardcoded classes.

[qiangxue]

Yes, this is better. Could you please double check to make sure it works with PHP 5.1?

[qiangxue]

One potential problem is that someone defines an abstract behavior class with non-empty methods which contain default implementation.

[creocoder]

@qiangxue Can we ignore this potential problem? Or we need other solution?

[Ragazzo]

@creocoder am, i got this one problem that @qiangxue discribed. I mean i have component which has defaults 3 behaviors, that has some defaults method, so i will get en error here?am i right?

[creocoder]

@Ragazzo This behaviors is abstract?

[Ragazzo]

@creocoder in on use-case yes(other behaviors in modules just overrides him - this one is abstract), and some other not, i've described them above, they just extending CBehavior. I think that your solution for memory consumption is good, can u then just describe how to fix this problem, if your solution will be accepted.

[creocoder]

@klimov-paul

Now about your controller example

I will see a fatal error: undeclarater method.

:-) No man. You just will see Unable to resolve the request "someController". You can try. So, implicit errors its all about Yii. And as i say this is OK. Just do not make them.

[klimov-paul]

All right, fine.

[serebrov]

Maybe a bit late, but I agree with @klimov-paul - changing method visibility from protected to public is not obvious and in my opinion it is a bad OOP practice. And Yii is a very good example of strong and correct OOP approach.

So I think that solution suggested by @slavcodev is better and fix issue for existing Yii classes.
As for Yii 2 - I think it is better to leave events() method empty. And the developer will be responsible for making a list of actually used events.

[grikdotnet]

Yii is whatever, but NOT a good OOP approach. It violates MVC principles and forces dependencies in so many places that I don't eve want to list them. Learn OOP and take it easy. Yii is what it is, and it is an OOP with a strong smell of spaghetti-joomla.

[serebrov]

@grikdotnet I can NOT imagine worse place then github issue to post your opinion about what Yii is.

[creocoder]

@sebgoo

changing method visibility from protected to public is not obvious and in my opinion it is a bad OOP practice

It is not bad practice in our case. Imagine that we hit in 1% of situations when changing method visibility is good and best choose we can do.

So I think that solution suggested by @slavcodev is better and fix issue for existing Yii classes.

Hardcoding childen classes in parent class is worst thing we can do from OOP point of view.

As for Yii 2 - I think it is better to leave events() method empty.

This is not DRY.

P.S. I hope you will understand our decision when to weigh the pros and cons.

[apptous-seb]

@creocoder I agree that current solution looks eleagant comparing to all other variants, but it is sill not perfect.
Any solution found here is a kind of hack and the best would be something that removes the root of the problem in behaviour classes and breaks BC.
If breaking BC is not an option then I would prefer a hack that stays hidden (most of the time) inside the framework code then hack which exposed to framework user.
So I do not compare solutions by "what is less bad programming practice" criteria, but by "what will fix problem and will be transparent for framework user most of time".
P.S. Anyway I understand that decision is made and will not argue anymore, just wanted to explain my opinion.
P.P.S. It is sebgoo (accidentaly posted under another account).

[creocoder]

@apptous-seb Visibility changing in children classes from strong to weak is not a hack.

[klimov-paul]

The solution breaks a BC!
In the Yii 1.1.12 any protected method can be assigned as event handler.
Try this:

class TestModel extends CFormModel {
    public $name;
    public function behaviors() {
        return array(
            'test' => array(
                'class' => 'TestModelBehavior'
            ),
        );
    }
}
class TestModelBehavior extends CBehavior {
    public function events() {
        return array(
            'onBeforeValidate' => 'beforeValidate'
        );
    }
    protected function beforeValidate() {
        die(__METHOD__);
    }
}
$model = new TestModel();
$model->validate();

I am not sure why such thing happens. I am using Yii 1.1.12 at PHP 5.3.3.
Can anyone confirm this?

[slavcodev]

@klimov-paul you are first, this is what I was waiting for that option with protected/public confuses many.
In behavior need to make public methods.

class TestModelBehavior extends CBehavior {
    public function events() {
        return array(
            'onBeforeValidate' => 'beforeValidate'
        );
    }

    public function beforeValidate() {
        die(__METHOD__);
    }
}

[creocoder]

@klimov-paul

The solution breaks a BC!
In the Yii 1.1.12 any protected method can be assigned as event handler.

Can't confirm. Anyway this solution not break BC. There is no places where:

I am not sure why such thing happens.

[serebrov]

@slavcodev , @creocoder The code by @klimov-paul works and this means that it is possible to use protected methods as event handlers.
So someone can already have such methods. Current solution will prevent attaching event handlers for protected methods and this will break BC for behaviors with protected event handlers.

@klimov-paul I think protected methods work because event handeling is in CComponent and CBehavior extends CComponent, so CComponent is able to call protected method of CBehavior instance.

[creocoder]

@sebgoo Someone can already have another BUGS in app. Who cares about it?

Event handlers should be PUBLIC.

[klimov-paul]

The example I have provided works at my server. The protected event handler is actually invoked!
Although it can be result of some extensions I use or PHP settings. I am not sure if this is a native PHP behavior.
But if it is, someone else may already use this. If protected method can be an event handler, someone may use this to hide event handlers from the direct invokes for object.

In my example "beforeValidate()" handles event, however

$behavior->beforeValidate();

will produce an error.

[klimov-paul]

This solution will produce a silent (without any message) error in case there is a proteceted event handler.

[creocoder]

@klimov-paul Congratulations. You have not found anything better than to look for what is non-existent in practice code.

[klimov-paul]

Perhaps I did.

[creocoder]

@klimov-paul :-) You cant change public to protected in children. Your example is specially forged. TestModelBehavior will extends CModelBehavior in real applications! So stop spam this thread with garbage non-existent code.

[serebrov]

@klimov-paul By the way your example works only if we extend CBehavior, but will fail with error if we extend CModelBehavior (can not change public to protected), so it is a very small chance to break someone's code.

[serebrov]

Another idea - detect empty event handlers based on method length:

public function attach($owner)
{
    $this->_owner=$owner;
    if(($events=$this->events())!==array()){
        $reflector=new ReflectionClass($this);
        foreach($events as $event=>$handler){
            $method=$reflector->getMethod($handler);
            $length=$method->getEndLine() - $method->getStartLine();
            if($length)
                $owner->attachEventHandler($event,array($this,$handler));
        }
    }
}

...
/**
 * PHP docs
 */
public function afterConstruct($event) {}

The only drawback I see is code formatting - we need to define empty handlers in single line to get zero length.
Update: and can break BC if someone have public function afterConstruct($event) { do_everything_in_one_line(); }

[creocoder]

@sebgoo You really think this solution is better??? More unreliable and ugly solution hard to think..

[serebrov]

@creocoder Just another option, don't be so angry ))) and, by the way, thanks for your efforts - you do a great job by finding and trying to fix such issues as this and I am not trying to reject it.

[cebe]

I see that this may cause problems with existing code in some rare cases. I think solution is good as it is but we should mention this change in UPGRADE notes. created an issue for that: #1550