[Mono-dev] Re: [Mono-devel-list] System.Data : DataTable.Select performance

[tsureshkumar]

@@ -193,9 +206,14 @@
  				return false;
  			}
-			if (_filter != filter)
-				return false;
+			if (_filter != null) {
+				if (!_filter.Equals (filter))
+					return false;
+			}
+			else if (filter != null)
+					return false;

=======================================================
please review this conditional checking.
=======================================================

 > @@ -640,6 +640,10 @@
 > Table.RecordCache.DisposeRecord(Proposed);
 >  				Proposed = -1;
 > +
 > +				int newVersion = (HasVersion > (DataRowVersion.Current)) ?
Current : Original;					
 > +				foreach(Index index in Table.Indexes)
 > +					index.Update(this,newVersion);					
=======================================================
why need to update indexes in CancelEdit. anyway, all values of the row
are removed?.
=======================================================

 > @@ -1299,7 +1303,13 @@
 >  				}
 >  				CheckChildRows(DataRowAction.Rollback);
 >
 > -				Current = Original;
 > +				if (Current != Original) {
 > +					foreach(Index index in Table.Indexes) {
 > +						index.Delete (this);
 > +						> index.Update(this,Original);
 > +					}
 > +					Current = Original;
 > +				}

=======================================================
that means, wherever there is a pattern like this (even Original =
Current), we need to update indexes.

ChangLog is also missing.
=======================================================

 > Hello all,
 > The example attached demonstrates the performance problem in
 > DataTable.Select - we run 30-40 times slower than .Net.
 > The cause for this problem is creating a new index for each select
 > executed without actually adding the newly created index into the table
 > indexes list.
 >
 > This patch improves a performance to be "only" 2 times slower than .Net.
 > Note that there is no performance improvement on the commented out
 > section since indexes are created per each different select expression.
 >

that is good.


 > There is two additional problems :
 > - Not that overall number of the rows returned differs from .Net. It is
 > a separate bug (it happens before the update also).

is there a bug report?

 > - As you can see,  MonoTests_System.Data.DataTableTest2.Select_ByFilter
 > fails fails with this fix. The failure is caused by the reason that the
 > previous test one throws an exception while executing Select() (as
 > supposed), thus leaving the index in the inconsistent state, so
 > additional Select() that reuses this index returns wrong result. The
 > problem is a bit more general - there are some more cases we suppose
 > index update or rebuild procedure to fail with exception, leaving index
 > in inconsistent state.

IMO, index should not become invalid. or when it becomes invalid, it
should auto correct itself.

 >
 > The question is what side we want to recognize the problem and to be
 > responsible for removing the broken index : the index or the callee?
 > Each of the solutions have both good and bad sides : solution on the
 > index side looks bad because index needs to intervent into its parent
 > object data structures and the callee side solution requires changes in
 > multiple places plus integration of the solution in each of the future
 > index uses.

Solution1 : delegate based mechanism to auto correct faulty indexes.
Solution2 : manually correct the index from the caller, when he finds
that index is wrong.

both are ok as long as we follow similar pattern for dealing with
indexes even though we repeat at many places.


regressions are not acceptable in favor of performance benefits. your3
patch introduces 1 regression in default profile and 2 regressions in
net_2_0 profile. But, the bug is somewhere with handling of the indexes.
  I guess that's why the indexes are not rebuilt with DataTable.Select
as well as DataRowCollection.Find method.

I think you can split your fix into

1. Equals implementations for expressions
2. Filter.Equals inside Key.Equals
3. Select method optimization.

We can check in 1 & 2 and delay 3 until the fix for bug. Let's keep the
architecture ready and we can optimize when needed.

Regards,
suresh.

 >
 > Any critics/ideas on the issue?
 >
 > Thanks,
 >
 > --
 > Boris Kirzner
 > Mono R&D team, Mainsoft Corporation.
 > Blogging at http://boriskirzner.blogspot.com/