Opened 3 years ago

Closed 2 years ago

#2270 closed defect (done)

Fix event handling in RunCollectionViews

Reported by: abeham Owned by: ascheibe
Priority: medium Milestone: HeuristicLab 3.3.12
Component: Optimization.Views Version: 3.3.10
Keywords: Cc:

Description

When a new result is added to a run, the RunCollection will fire a Reset event which will trigger a complete refresh of all dependent views. The suppressEvents field is not used to cut short on batch updates.

This can be easily fixed in the bubble chart view, but not so easy to fix in the table view. There it would require to make methods from StringConvertibleMatrixView protected virtual.

I had a case where I added a new result to more than 2000 runs and it took at least 5 minutes. With a bit of change that was a matter of not even seconds.

Attachments (1)

runcollectionview.patch (2.4 KB) - added by abeham 3 years ago.
Fixes some performance issues for bubble chart and table

Download all attachments as: .zip

Change History (22)

Changed 3 years ago by abeham

Fixes some performance issues for bubble chart and table

comment:1 Changed 3 years ago by abeham

  • Owner set to mkommend
  • Status changed from new to assigned

Please have a look at the attached patch. I didn't look into boxplot and other runcollection views.

comment:2 Changed 3 years ago by mkommend

  • Milestone changed from HeuristicLab 3.3.11 to HeuristicLab 3.3.12

comment:3 Changed 2 years ago by mkommend

  • Owner changed from mkommend to abeham

The changes in the attached patch look fine. I had a quick look at the boxplot view, runcollection view, and chart aggregation view and i think those must be adapted, because the suppress events flag is not used when a reset event is fired.

comment:4 Changed 2 years ago by abeham

r12077: Added suppressUpdates check for Content_Reset events

Chart aggregation and runcollection view aren't affected because they don't listen to Content_Reset. I noticed however that chart aggregation view threw an exception in the test below, but I haven't had time to investigate that yet. (And chart aggregation should probably listen for Content_Reset also).

Here is some code for testing. Execute it once to get a runcollection with 1000 runs, then open a view on that, put it side by side to the script and execute the script again.

using System;
using System.Linq;
using System.Collections.Generic;

using HeuristicLab.Core;
using HeuristicLab.Common;
using HeuristicLab.Collections;
using HeuristicLab.Data;
using HeuristicLab.Optimization;
using HeuristicLab.Analysis;
using HeuristicLab.MainForm;
using HeuristicLab.MainForm.WindowsForms;

public class MyScript : HeuristicLab.Scripting.CSharpScriptBase {
  public override void Main() {
    RunCollection runs;
    if (!vars.Contains("runs")) {
      runs = new RunCollection();
      AddRuns(runs, 1000);
      vars.runs = runs;
      var before = ((HeuristicLab.MainForm.WindowsForms.DockingMainForm)MainFormManager.MainForm).ShowContentInViewHost;
      ((HeuristicLab.MainForm.WindowsForms.DockingMainForm)MainFormManager.MainForm).ShowContentInViewHost = false;
      foreach (var rcviewType in MainFormManager.GetViewTypes(typeof(RunCollection))) {
        MainFormManager.MainForm.ShowContent(runs, rcviewType);
      }
      ((HeuristicLab.MainForm.WindowsForms.DockingMainForm)MainFormManager.MainForm).ShowContentInViewHost = before;
    } else {
      runs = (RunCollection)vars.runs;
      try {
        runs.UpdateOfRunsInProgress = true;
        AddResult(runs);
      } finally { runs.UpdateOfRunsInProgress = false; }
    }
  }
      
  private void AddRuns(RunCollection runs, int count) {
    var random = new System.Random();
    for (var i = 0; i < count; i++) {
      var run = new Run() { Name = "This is run " + i + " (" + random.Next(10000) + ")" };
      run.Parameters.Add("Param1", new IntValue(i % 20));
      run.Results.Add("Result1", new DoubleValue(random.NextDouble()));
      runs.Add(run);
    }
  }
  
  private void AddResult(RunCollection runs) {
    var random = new System.Random();
    foreach (var run in runs) {
      run.Results.Add("Result" + (run.Results.Count + 1), new DoubleValue(random.NextDouble()));
    }
    foreach (var run in runs) {
      var dt = new DataTable("Bla");
      dt.Rows.Add(new DataRow("Rowrow", "It's a row", Enumerable.Range(0, 20).Select(x => random.NextDouble() * x)));
      run.Results.Add("Table" + (run.Results.Count + 1), dt);
    }
  }
}

Last edited 2 years ago by abeham (previous) (diff)

comment:5 Changed 2 years ago by abeham

  • Status changed from assigned to accepted

should also accept ticket...

comment:6 Changed 2 years ago by abeham

r12599: Fixed several of the runcollection views

  • Added InvokeRequired checks where missing
  • Added suppressUpdates check where missing
  • Fixed some bugs, added some null checks

Please fix the remaining bugs in the statistical analysis view. Please also take a look into the chart analysis and aggregation views to see why they don't notice the DataTable that was added to the results after the view was created.

comment:7 Changed 2 years ago by abeham

  • Owner changed from abeham to ascheibe
  • Status changed from accepted to assigned

comment:8 Changed 2 years ago by ascheibe

  • Status changed from assigned to accepted

comment:9 Changed 2 years ago by ascheibe

r12613 fixed NullReferenceException in statistical tests view by introducing a flag to suppress firing ui events

comment:10 Changed 2 years ago by ascheibe

Commits from #2348, also to be released with this ticket: r12112, r12116, r12117, r12131.

comment:11 Changed 2 years ago by ascheibe

  • Owner changed from ascheibe to abeham
  • Status changed from accepted to reviewing

r12631 added refresh of data table combobox for the chart analysis and aggregation view

comment:12 Changed 2 years ago by abeham

  • Owner changed from abeham to ascheibe
  • Status changed from reviewing to assigned

r12672: Removed firing of ToStringChanged together with ItemChanged or Reset, ToString never changes for run collections

Please make sure that statistical analysis view listens to changed results or parameters.

comment:13 Changed 2 years ago by abeham

  • Summary changed from Slow reaction of runcollection views when runs change to Fix event handling in RunCollectionViews

comment:14 Changed 2 years ago by abeham

Release together with #2354

comment:15 Changed 2 years ago by ascheibe

  • Status changed from assigned to accepted

r12684 statistical analysis view updates now properly when the runcollection is changed

comment:16 Changed 2 years ago by ascheibe

  • Owner changed from ascheibe to abeham
  • Status changed from accepted to reviewing

comment:17 Changed 2 years ago by abeham

Line 385 in StatisticalTestsView throws an InvalidOperationException due to cross-threading issues:

pValTextBox.Text = pval.ToString();

Line 226 in SampleSizeInfluenceView also cause cross-thread exceptions:

string selectedYAxis = (string)this.yAxisComboBox.SelectedItem;

comment:18 Changed 2 years ago by ascheibe

r12690 fixed cross-threading issues in run collection views

comment:19 Changed 2 years ago by abeham

r12692:

  • Fixed bug in RunCollectionChartAggregationView regarding null value
  • Fixed bug in RunCollectionTableView when runs were added during UpdateOfRunsInProgress
  • Added suppressUpdates to RunCollectionView and listening to UpdateOfRunsInProgress
  • Made sure that RunCollection would reset updateOfRunsInProgress to the same value as before when changing it in internal methods

comment:20 Changed 2 years ago by abeham

  • Owner changed from abeham to ascheibe
  • Status changed from reviewing to readytorelease

I made a couple of additional tests. I think we can release this ticket.

comment:21 Changed 2 years ago by ascheibe

  • Resolution set to done
  • Status changed from readytorelease to closed
Note: See TracTickets for help on using tickets.