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

Improve performance massively by fixing a stupid binding mistake #6316

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

tobiasdiez
Copy link
Member

There was a really stupid mistake in the way the main table was set up: the bindings for converting the raw field value to the proper displayed representation were created in the setCellValueFactory.

setCellValueFactory(param -> getColumnValue(param.getValue()));

return Bindings.createStringBinding(() -> computeText(entry), dependencies);

This has the consequence that whenever the table view asked for the current value a new binding was created and the value was re-calculated over and over again. For example, if there were to the entry list then the table view asks all entries if their values changes. Usually only a couple of entries are changed, so that only these entries need to be redrawn. But since the binding was re-created, actually all entries were redrawn instead.
Moreover, since every call to cell value factory created a new binding, a new invalidation listener was added to fields every time, leading to really high numbers of a few thousand invalidation listeners.
I really hate myself for this stupid implementation 🙈 .

This PR fixes this problem by simply creating the bindings for each entry once, and then reuse. This lead to a massive performance increase as well as a huge memory consumption improvement. For example, sorting and filtering using groups in @AEgit huge database is now almost instant. Moreover, the memory is down from 2gb to a bit more than 1gb (this tells you how many useless invalidation listeners were there before).

Should fix the renaming issues in #5071. Might fix #5919 as well as #5735.

  • Change in CHANGELOG.md described (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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 19, 2020
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will test this, but Code wise looks good!
Kudos for spotting and fixing the bug!

@Siedlerchr
Copy link
Member

Startup still takes some time, but it's already faster than before with the huge database.

@Siedlerchr Siedlerchr merged commit bc57d22 into master Apr 20, 2020
@Siedlerchr Siedlerchr deleted the bindingPerf branch April 20, 2020 06:25
Siedlerchr added a commit that referenced this pull request Apr 24, 2020
* upstream/master:
  Update label names
  Squashed 'src/main/resources/csl-locales/' changes from d0ee4d13c9..79845b087b
  Squashed 'src/main/resources/csl-styles/' changes from c1793d2..143464e
  Cite work by @ayaankazerouni
  Improve performance for loading files (#6332)
  Add ADR von JUnit vs. AssertJ (#6335)
  'Name' textfield on focus instead of 'OK' button when user clicks on 'Add subgroup' option (#6330)
  Bump jurt from 6.3.2 to 6.4.3 (#6325)
  Bump unoil from 6.3.2 to 6.4.3 (#6320)
  Bump ridl from 6.3.2 to 6.4.3 (#6326)
  Bump classgraph from 4.8.69 to 4.8.71 (#6322)
  Bump flexmark from 0.61.6 to 0.61.16 (#6318)
  Bump richtextfx from 0.10.4 to 0.10.5 (#6319)
  Bump guava from 28.2-jre to 29.0-jre (#6323)
  Bump flexmark-ext-gfm-tasklist from 0.61.6 to 0.61.16 (#6327)
  Bump org.beryx.jlink from 2.17.5 to 2.17.7 (#6324)
  Improve performance massively by fixing a stupid binding mistake (#6316)
  Fixed missing paste command (#6313)
  Remove cache of auto completion results (#6310)
Siedlerchr added a commit that referenced this pull request Apr 26, 2020
# By Tobias Diez (10) and others
# Via dimitra-karadima (5) and others
* upstream/master: (62 commits)
  Backward compatibility for 4.3.1 (#6296)
  Fix Export to clipboard Dialog icon (#6345)
  Refactor EntryEditorPreferences (#6245)
  Update label names
  Squashed 'src/main/resources/csl-locales/' changes from d0ee4d13c9..79845b087b
  Squashed 'src/main/resources/csl-styles/' changes from c1793d2..143464e
  Cite work by @ayaankazerouni
  Improve performance for loading files (#6332)
  Add ADR von JUnit vs. AssertJ (#6335)
  'Name' textfield on focus instead of 'OK' button when user clicks on 'Add subgroup' option (#6330)
  Bump jurt from 6.3.2 to 6.4.3 (#6325)
  Bump unoil from 6.3.2 to 6.4.3 (#6320)
  Bump ridl from 6.3.2 to 6.4.3 (#6326)
  Bump classgraph from 4.8.69 to 4.8.71 (#6322)
  Bump flexmark from 0.61.6 to 0.61.16 (#6318)
  Bump richtextfx from 0.10.4 to 0.10.5 (#6319)
  Bump guava from 28.2-jre to 29.0-jre (#6323)
  Bump flexmark-ext-gfm-tasklist from 0.61.6 to 0.61.16 (#6327)
  Bump org.beryx.jlink from 2.17.5 to 2.17.7 (#6324)
  Improve performance massively by fixing a stupid binding mistake (#6316)
  ...

# Conflicts:
#	build.gradle
Siedlerchr added a commit that referenced this pull request Apr 30, 2020
…ionCaseInsensitive

* upstream/master: (32 commits)
  Fix Export to clipboard Dialog icon (#6345)
  Refactor EntryEditorPreferences (#6245)
  Update label names
  Squashed 'src/main/resources/csl-locales/' changes from d0ee4d13c9..79845b087b
  Squashed 'src/main/resources/csl-styles/' changes from c1793d2..143464e
  Cite work by @ayaankazerouni
  Improve performance for loading files (#6332)
  Add ADR von JUnit vs. AssertJ (#6335)
  'Name' textfield on focus instead of 'OK' button when user clicks on 'Add subgroup' option (#6330)
  Bump jurt from 6.3.2 to 6.4.3 (#6325)
  Bump unoil from 6.3.2 to 6.4.3 (#6320)
  Bump ridl from 6.3.2 to 6.4.3 (#6326)
  Bump classgraph from 4.8.69 to 4.8.71 (#6322)
  Bump flexmark from 0.61.6 to 0.61.16 (#6318)
  Bump richtextfx from 0.10.4 to 0.10.5 (#6319)
  Bump guava from 28.2-jre to 29.0-jre (#6323)
  Bump flexmark-ext-gfm-tasklist from 0.61.6 to 0.61.16 (#6327)
  Bump org.beryx.jlink from 2.17.5 to 2.17.7 (#6324)
  Improve performance massively by fixing a stupid binding mistake (#6316)
  Fixed missing paste command (#6313)
  ...
koppor pushed a commit that referenced this pull request Apr 15, 2023
4728902faa Create wiley-was.csl (#6283)
5853909f89 Create journal-of-human-nutrition-and-dietetics.csl (#6311)
286a3606c1 Create stanovnistvo.csl (#6431)
22a7139dc5 Update zitierguide-leitfaden-zum-fachgerechten-zitieren-in-rechtswiss… (#6489)
e51e9363ee Update netherlands-journal-of-geosciences-geologie-en-mijnbouw.csl (#6493)
4e1a69231c Add Bristol UP dependents (auto-generated) (#6494)
c9021437a4 Create microbiome-research-reports.csl (#6371)
f71e516dda Update harvard-anglia-ruskin-university.csl (#6365)
c187663e75 Create publicatiewijzer.csl (#6316)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 4728902faa1ef43c0d89366c77b7ea8f46c6730f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JabRef 5 on Linux x86_64 Extreme Memory Pressure, System Hangs, Unusable
2 participants