TableFilter Performance Optimizations

#581 Merged at 4f0c06a
Repository
Deleted repository
Branch
tablefilter_optimized (2e105b7a3972)
Repository
ControlsFX
Branch
default
Author
  1. Thomas Nield
Reviewers
Description

Overhauling the internal engine with TableFilter to be more event-driven to maximize efficiency and improve performance.

This is a work-in-progress.

Comments (17)

  1. Thomas Nield author

    You can also check out the LargeTableFilterTest to get a quick idea on how it performs. It is definitely improved although I would like to identify a few bottlenecks in some cases.

  2. Thomas Nield author

    Bam! Lightning fast now. Filtered 20,000 records in less than two seconds. Okay, I'm done for now. Feel free to take a look and I'll follow up tomorrow.

  3. Thomas Nield author

    Okay maybe not lightning fast, but relatively it is a huge improvement to what it was before : ).

    I'll be doing a code review later to see if there are other improvements I can do. It already does things like checking if any values are unselected in a column before qualifying. I eliminated all "scanning" behaviors (that I know of) and instead built event-driven HashMaps.

  4. Thomas Nield author

    Hard part is done. I was able to implement cell value listeners. I would like to get the distinct values to sort, as well as fix some issues with the graying out of values.

  5. Thomas Nield author

    Sorry for spammy messages. All cited issues above have been fixed. I have tested the heck out of this thing. Take a look and let me know what you think.

  6. Thomas Nield author

    Okay this is working awesome but since I'm pushing this to the highest quality possible, I want to resolve a problem with concurrent cell value updates (especially since I want this to play nice with RxJavaFX). I may need some thoughts here and yes, all updates are happening on the JavaFX thread :)

    I attached a ConcurrentTableFilterTest application class which will schedule values in one column to be updated on a fixed ExecutorService. If you filter any columns while these values are updating, it will likely throw an IllegalStateException that I have deliberately declared in the DupeCounter<T> class.

    The core logic is supported by ColumnFilter<T,R> and DupeCounter<T>, the latter which keeps track of duplicate counts for each distinct value. If a count for a given value becomes 1, that value is added to a collection of distinct values. If the count drops to 0, it will remove that item from the distinct values. Going below 0 will logically throw an IllegalStateException as this indicates it tried to remove something that is no longer existent, which means something went wrong with the tracking.

    Two instances of DupeCounter are used, one to support the management of distinct filter values, and the other to keep track of distinct "visible values" (which drive the grayed out values when they fall out of scope).

    For some reason, the "visible" distinct values are struggling with these cells updating concurrently because it tries to drop the DupeCounter below to 0, which throws an IllegalStateException. I was tempted to throw out this check but I really want to understand why it is doing it first. I have confirmed there is no race condition and everything is happening on the JavaFX thread. Does anybody want to help code review this one behavior? This code is not as beastly as it used to be.

  7. Thomas Nield author

    Okay I figured it out. The IllegalStateException should not be thrown when it comes to driving the grayed out values as they fall out of scope. This check is irrelevant and illogical because if a cell value is updated by a background process, but that cell is hidden from view due to an existing filter, the cell's visibility is already irrelevant. But trying to remove the hidden cell's old value from the DupeCounter will cause the count to go below 0.

    So I just disabled the DupeCounter's IllegalStateException for the grayed-out values feature. It is still in place for the distinct filter value management. Classifying this as a problem would be pretty perfectionist and not worth the effort for now.

    The impact of not addressing is this: having an existing filter applied, if another column's values are changing the grayed out mechanism may not be accurate for that column. Like I said, it's a perfectionist pursuit.

    filter values.png

    So I'm done for real this time. I encourage checking out the ConcurrentTableFilterTest to see a demonstration of the dynamic updating. Please merge whenever you are ready. Let me know if you have any questions or comments. Overall, I'm much happier with this control and there should be a real difference in performance and quality, especially with automatic accounting for cell values changing.

  8. Thomas Nield author

    Of course, the sample UI's are all in there, including the ConcurrentTableFilterTest.

    There is one more complication I found and I might be creating a memory leak. When I call TableColumn#getCellObservableValue I assumed it returned a stored, reused instance rather than a new one. I don't think this is the behavior, so the way I track the listeners with a HashMap<CellIdentity<R>,ChangeListener<R>> is completely useless, where CellIdentity<R> is this:

     private static final class CellIdentity<R> {
            private final ObservableValue<R> cellValue;
    
            CellIdentity(ObservableValue<R> cellValue) {
                this.cellValue = cellValue;
            }
    
            @Override
            public boolean equals(Object other) {
                return this.cellValue == ((CellIdentity<?>)other).cellValue;
            }
    
            @Override
            public int hashCode() {
                return System.identityHashCode(cellValue);
            }
        }
    

    I'm trying to figure out how I can add listeners to a cell's ObservableValue and have a way to remove the listener the moment the backing record is removed. The code in question is here
    https://bitbucket.org/thomasnield/controlsfx/src/23734cf97755f38deefd9d3dd1e46e93741a764d/controlsfx/src/main/java/impl/org/controlsfx/table/ColumnFilter.java?fileviewer=file-view-default#ColumnFilter.java-136:174

  9. Thomas Nield author

    We should be good here. I've been using this with my complex concurrent application for several days and have had no issues. You are welcome to code review and merge when you're ready.