Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent the EventBus from triggering searches #10619

Merged
merged 13 commits into from
Nov 10, 2023
Merged

Conversation

k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 commented Nov 3, 2023

Fixes a performance problem while editing a BibEntry while sorting/filtering the maintable

A few months ago, we discussed a replacement for the EventBus, and I believe we didn't disagree that JavaFX has largely usurped its role.
The performance issue is most likely that the events on the EventBus triggered a full search AND filtering on every update.
This PR removes the EventBus code, as the same functionality is implemented using JavaFX.
While I haven't verified that this is the case, in theory, the JavaFX code is more efficient and doesn't require full filtering and sorting of the MainTable on every change.

At worst, the changes should be twice as fast. At best, it is a lot faster.

Resolves #8977.

Todo

  • Removes filtering on events that use the EventBus
    • They caused a full filtering and sorting event on every BibEntry modification
      • Verify statement in a running copy of JabRef
      • Verify that at most one element gets filtered on changes to a field
      • Verify that sorting the list after one element is changed is inexpensive
      • Can I write tests for this? 😀
  • Don't filter the entries if the search query isn't updated (statemanager.setSearchQuery)
    • Requires .equal. If we want this we should onboard Lombok or a similar tool.
  • Reduce the scope of PR by creating a new GH issue for the UI problem
    • Prevent the BibEntry that is being actively edited from being filtered
      • When a BibEntry is no longer being edited, we perform another filtering based on the search query
        • GroupTreeView & GlobalSearchBar: FxTimer.create(Duration.ofMillis(400), _)
        • When the EntryEditor is closed or changes BibEntry?

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 changed the title Remove updating search on EntryChangedEvent Prevent search results from updating on EntryChangedEvent Nov 3, 2023
@JabRef JabRef deleted a comment from github-actions bot Nov 3, 2023
@JabRef JabRef deleted a comment from github-actions bot Nov 3, 2023
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

k3KAW8Pnf7mkmdSMPHz27 commented Nov 4, 2023

  1. The problem is that we want the filtering based on search queries to only occur on updates to the query

    1. Cannot be done with FilteredList; it updates on changes to underlying observables
  2. We want the filtered result based on groups to update when BibEntries are updated

    1. Should be done with FilteredList
  3. Filtering is an O(n) operation. If this is the performance bottle-neck, it must be that applying the filter to one BibEntry must be very expensive, probably performing the filtering operation on all fields of the BibEntry

    1. In theory, filtering should be in O(1) and only performed on the changed element
      1. Verify the above statement in the source code
    2. Filtering on Groups should be inexpensive, and if it isn't already cached, we can cache intermediate results
      1. Can we modify it to only listen to updates to one field without it severely impacting the code quality?
        1. Premature optimization
    3. When do we filter the list from scratch based on the search query? It seems to be very expensive
  4. Sorting is in O(nlogn) but probably only on one partial field -> inexpensive

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

k3KAW8Pnf7mkmdSMPHz27 commented Nov 4, 2023

Ok. Almost nothing works because,

  1. The ObservableList that we use creates update events on changes to all BibEntry fields in addition to list permutations/resizing. We want this behavior.
  2. What we are trying to do is probably against the principles of JavaFX, i.e., micro-control Change and force a stale state.
    1. Try to keep the BibEntry that is being actively edited, even if it is rejected by the query, see Prevent the EventBus from triggering searches #10619 (comment)

Next attempts,

  1. Port mapMulti to EasyBind. That is probably going to be nasty, and I am not 100% sure that it will work
    1. ReactFX serves this purpose better
      1. Filtered ListChange streams using ReactFX can work for manually adding/removing items from a wrapped ObservableArrayList, but the bookkeeping can be nasty
  2. Extend TransformList and re-implement most of the behavior of FilterList
    1. Wrapping FilterList is not going to help, and it is a final class
    2. It seems better to check the JavaFX license and see if we can copy FilterList and modify it, at that is a very bad thing to be "better"
  3. Create a partial bind for ObservableList that only cares about add/remove/permute. This is IMHO a very ugly solution
  4. Review the performance of the default search, it might be time better spent :(

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

We might have done searches twice before, doubt it but verify

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

I think I have cracked the JavaFX way of solving this! We need MORE bindings and observables!!!!!!!!!! 😲

  1. Adapt the CoarseFilter to JavaFX and/or add/use an observable for when an entry is being edited in the Statemanager or LibraryTab
  2. Cache the search query so it doesn't by necessity force a search everytime performSearch is called
  3. Adapt the FilteredList to always keep "accept" the BibEntry that is being edited unless the search query has been updated -> stateful filtering

Copy link
Contributor

github-actions bot commented Nov 4, 2023

While the PR was in progress, JabRef released a new version. You have to merge upstream/main and move your entry in CHANGELOG.md to section ## [Unreleased].

It might also be that another CHANGELOG.md issue arose. You can check the detailed error output at the tab "Checks", section "Tests" (on the left), subsection "CHANGELOG.md".

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

k3KAW8Pnf7mkmdSMPHz27 commented Nov 4, 2023

  1. In theory, the filtering should only update the one item in the list, which means it should be in effect independent of the cost of filtering. How do I verify this?
    1. The filtering event will cause a shift in an Array, which is a contiguous memory copy. It should be a cheap operation compared to resorting.

What can trigger filtering on the complete list? A field modification?

Searchbar behavior options:

  1. When the search bar loses focus, we can choose to stop filtering based on the search query
  2. When a BibEntry is no longer being edited, we perform another filtering based on the search query
    1. How do you define an editing event as being over?
      1. How do we define it in other parts of the code?
        1. GroupTreeView & GlobalSearchBar: FxTimer.create(Duration.ofMillis(400), _)
        2. CoarseFilter
      2. EntryEditor is closed or changes BibEntry?
        1. Can I even check this?

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

k3KAW8Pnf7mkmdSMPHz27 commented Nov 4, 2023

Based on recent activity in the issue, @gianlucabaldassarre @teertinker @ytzemih @m-mauersberger could any of you try if the problem is reduced in the latest build of this PR? At the time of writing it is this one. Beware that there might be bugs and odd behaviors, preferably backup your library before trying.

Note that the changes to the filtering behavior are unlikely to be done by the time you try it, but hopefully, there will be some performance improvements.

@ytzemih
Copy link

ytzemih commented Nov 5, 2023

Thanks, @k3KAW8Pnf7mkmdSMPHz27 for taking care of this issue. I tried the PR, you pointed me to, and it seems as if the problem is fixed. Editing an entry can be done without delayed typing while the search field is non-empty (i.e. the search filter is active). Looking forward to seeing this transferred to the next stable release. Thanks again. Hope others can confirm my observation.

@teertinker
Copy link
Contributor

Hey, I did try it on my slowest machine. It works very well. A great improvement for me.

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 changed the title Prevent search results from updating on EntryChangedEvent Prevent the EventBus from triggering searches Nov 5, 2023
@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as ready for review November 5, 2023 21:28
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

I am suspecting

33:1  error  Change in a released version                                        releasechangesfrozen

Is incorrect. I'll look into it a bit later, I don't have any experience with heylogs, and IMO the changelog is correct.

@ytzemih
Copy link

ytzemih commented Nov 9, 2023

I've added an observation #8977 (comment) that may be relevant to the handling of this PR.

@Siedlerchr
Copy link
Member

Please also test with multiple open libraries. We need to make sure that the global search and the search across libraries also still works

@ytzemih
Copy link

ytzemih commented Nov 9, 2023

@Siedlerchr, I did. I'm usually using 4-6 databases at the same time. Whether or not global search is on does not make much difference (both works btw.) but what really is a show stopper, particularly, in large databases is when the renderer has to render an entry with some text in the "abstract" and/or "comment" field. I'm making comments a lot, increasingly also with itemized lists.

So, JR's speed seems to negatively correlate with DB size and use of abstract/comment field in entries.

I can imagine, if each single key stroke triggers a regexp search in a large database and(!) a (markdown-capable?) entry renderer, there is a hell lot going on.

@teertinker
Copy link
Contributor

Working with multiple libraries works for me without any problems. Global search works as expected. I can't see any difference with regard to search to the stable version.

I did also try to edit a small database with search on/off. I did not experience any issues. Jabref was responsive and fast.

Copy link
Contributor

github-actions bot commented Nov 10, 2023

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

@Siedlerchr Siedlerchr merged commit a1d783f into main Nov 10, 2023
12 of 16 checks passed
@Siedlerchr Siedlerchr deleted the fist-attempt-at-8977 branch November 10, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The process of typing into fields is too slow in a large database. Caused by sorting columns in main table.
5 participants