Fix group search performance #3553
Conversation
There was a problem hiding this comment.
The delayed search is a good idea and the code related to it looks fine to me.
A few days ago I also had a look at the stupid search algorithm, which tests children again and again. While this is definitely a brute force method, I don't think it is the reason for the performance problems. In the end the test is String.contains, which runs in a few nano-seconds for our small string sizes. So you have to run it a few 10k times in order to get a noticeable delay, which is hard to reach even if the algorithm is multiplicative in the size of children. I suspect that the main problem is the use of the childrenFactory in the showNode method. It means that in addition to this brute-force algorithm we are also recreating the tree over and over again instead of just traversing it. The creation of nodes is expensive since the hit counter triggers a run over all entries. @halirutan based on your experience with the performance investigation, is my observation correct?
| viewModel.filterTextProperty().bind(searchField.textProperty()); | ||
| // We try to to prevent publishing changes in the searchfield directly to the search task that takes some time | ||
| // for larger group structures. | ||
| final Timer searchTask = FxTimer.create(Duration.ofMillis(400), () -> { |
There was a problem hiding this comment.
I think, it makes sense to extract this code to a helper method in BindingsHelper.
| // We try to to prevent publishing changes in the searchfield directly to the search task that takes some time | ||
| // for larger group structures. | ||
| final Timer searchTask = FxTimer.create(Duration.ofMillis(400), () -> { | ||
| LOGGER.info("Run Search " + searchField.getText()); |
There was a problem hiding this comment.
I think debug level is enough.
There was a problem hiding this comment.
That log is really only for testing so that I could inspect when a new search was triggered and if my "reset the timer on new key" does actually work. This will be removed.
There was a problem hiding this comment.
Can the log level be changed to "debug" and the logging settings adjusted accordingly for you?
I guess, this is what I saw this morning in the profiler. I didn't investigate further, but I was surprised that I saw constructor-hits. I thought the group-tree is static until it is changed by adding/deleting nodes but instead it seems the tree is rebuilt. I haven't worked through the whole implementation because there is a lot of stuff in it I haven't worked with before JabRef. This is another reason why I had the hope someone could join on this branch because I don't feel confident to implement a good fix for this fundamental flaw in the design. |
koppor
changed the title
tobiasdiez
changed the title
tobiasdiez
added
the
status: ready-for-review
|
Ok, I think I found a solution which reuses the children, does not need to store the predicate but nonetheless has a good performance. |
There was a problem hiding this comment.
I have tested this locally with our large testing library. The improvement is really impressive, great work :) I have to say that the bindings code is quite hard to get, as always, but since it seems to work I am happy with.
I think this needs a changelog entry (it's a change that users will notice) and then it can be merged.
|
@lenhard Have you seen the comment in the issue this PR should solve?
|
|
@halirutan Sorry, I did not look at the issue again and just tested for performance, not correctness. Of course that bug should be fixed before merging. |
|
Bug fixed -> merge! 😃 |
|
Great! |
This includes currently 2 changes:
Fixes #2852
gradle localizationUpdate?