Bug 327684

Comment 1 Florian Pirchner 2010-10-13 12:19:43 EDT

The unit test:
public class TextBindingTest {

	private Shell shell;

	@Test
	public void test_SetBinding_BeforeSetBean_Without_Redview() {
		shell = new Shell(Display.getDefault(), SWT.NONE);
		Text text = new Text(shell, SWT.NONE);
		ITextRidget textRidget = (ITextRidget) SwtRidgetFactory
				.createRidget(text);

		StringBean bean = new StringBean();
		// first setup
		textRidget.bindToModel(bean, StringBean.PROP_VALUE);
		textRidget.updateFromModel();

		for (int i = 0; i < 1000; i++) {
			Date start = new Date();
			bean = new StringBean();
			textRidget.bindToModel(bean, StringBean.PROP_VALUE);
			textRidget.updateFromModel();
			Date end = new Date();
			long time = end.getTime() - start.getTime();
			if (time > 20) {
				Assert.fail(String.format("Took to long. %s runs!", i + 1));
			}
		}
	}

Comment 2 Steffen Kriese 2010-10-14 02:20:23 EDT

In my opinion rebinding a model indicates a programming error in the application. If you want to rebind a model you should bind to a facade and exchange the model behind the facade.

Example:

public class Person{
   private String name;
   ...
   public String getName(){
    return name;
   }
   ....
}

public class PersonFacade{
  private Person person;
  ...
  public String getName(){
    return person.getName();
  }
  
  
  public void setPerson(Person person){
   this.person = person;
  }
  
  ...
}

Comment 3 Florian Pirchner 2010-10-14 09:19:10 EDT

Hi Steffen,

of course, you are right for hand coded applications.

But if you are using a generic binding approach, the requirement of rebinding a model to the ridget does not have necessarily to be based on an programming error. In my opinon (2 years ago) this way was the most efficient way of rebinding a model. A generic facade implementation is much more expensive because you need an additional databinding context for your facade and have to use reflection to delegate getters and setters.

Today i thought about how i am able to implement a very efficient facade which allows me to use a generic binding information for all kind of bindings. Since the way how redView allows to deal with any kind of beans has dramatically changed in the last 2 years, i am able to build a very efficient facade without a databinding context and nor the use of reflection.
Right now i am not sure if it is possible to implement one for all kinds of ridgets. Specially the SelectableRidgets??!! I will see...


But nevertheless; the described problem of increasing response times to bind a model to the ridget seems to be a bug. I had another look and found out, that the databinding context seems to add a lot of listeners each time the bind()-method is invoked. In my case about 1.000 listeners had been added in org.eclipse.core.databinding.observable.ChangeManager, which caused the real high response times.


In summary:
Thanks for the hint according the facade. Will allow redView to be more efficient after the refactoring.
And please have a look at the binding problem.

Thanks, Flo

Comment 4 Elias Volanakis 2010-10-14 16:34:04 EDT

Hi Flo,

this is good feedback. If there is performance degrations, it propably means that there is also waster memory (listeners hanging around) so we should look into it. If disposing the DataBindingContext helps, then we could propably easily do that. 

Elias.

Comment 5 Florian Pirchner 2010-10-15 07:53:16 EDT

Hi Elias,

thanks in advance. So i am having a fallback if the instantiation of a generic facade is to resource costly.

Actually i am trying to find a generic facade implementation for all bindings.

thanks Flo

Comment 6 Elias Volanakis 2010-12-08 21:50:24 EST

Investigating.

Comment 7 Elias Volanakis 2010-12-15 16:51:11 EST

Excellent work Florian! Thank you.

By the way: I've narrowed down the issue to the fact that AggregateValidationStatus is not disposed (in rebindToModel()).

Fixed in HEAD.

Comment 8 Elias Volanakis 2010-12-15 17:14:39 EST

Created attachment 185275 [details]
Patch

Already committed - attaching just for reference.

Comment 9 Florian Pirchner 2010-12-16 02:25:44 EST

Hi Elias,

thanks a lot. 

Cheers, Flo

Comment 10 Christian Campo 2011-02-03 02:47:15 EST

I believe there is an issue left with this bug. @Elias can you please investigate....

One of the tests for this is I believe ValueBindingSupportTest in method testRebindToModelNoPerfomanceDegratation

The build server throws 
junit.framework.AssertionFailedError: junit.framework.AssertionFailedError: null
	at org.eclipse.riena.ui.ridgets.ValueBindingSupportTest.testRebindToModelNoPerfomanceDegratation(ValueBindingSupportTest.java:408)
	at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
	at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62)
	at org.eclipse.pde.internal.junit.runtime.UITestApplication$1.run(UITestApplication.java:116)
	
At times (not always).

The assert is this:
		assertTrue((time2 / 2) <= time1); (line 408)
		
So I believe that means that rebinding gets still slower by the factor of 2 after awhile (sometimes).

Comment 11 Elias Volanakis 2011-02-08 19:02:18 EST

Not sure how to reproduce this. If it happens on my machine, I'll have a look at it. 

At least I've changed the assert error message to print out the measured times.

Comment 12 Elias Volanakis 2011-02-28 18:15:42 EST

@Christian: Haven't see this in a while. Reopen if it happens again.