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

Optimize saving #7568

Merged
merged 7 commits into from
Mar 29, 2021
Merged

Optimize saving #7568

merged 7 commits into from
Mar 29, 2021

Conversation

Ali96kz
Copy link
Contributor

@Ali96kz Ali96kz commented Mar 23, 2021

#7265 Changed to simpleDateFormatter it doesn't throw a lot of exception

  • 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 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.

@Siedlerchr
Copy link
Member

Now this is not a good idea, you are effectively switching back to the legacy api 👎
Do the exceptions really matter? They are ignored anyways

@Ali96kz
Copy link
Contributor Author

Ali96kz commented Mar 24, 2021

@Siedlerchr DateTimeFormatter throw a lot of exception, that's why it leads to OOM. Regex validation is too complicated and will less efficiently than these solution

@Ali96kz
Copy link
Contributor Author

Ali96kz commented Mar 24, 2021

The same problem is here #6057

@calixtus
Copy link
Member

Hi @Ali96kz , first of all, thanks for your interest in JabRef and the issue triangulation. Since you found the most probable source of the issue, we can think about a fix.

Secondly, I have to agree with @Siedlerchr that switching back to the old 1.1 Java DateTime API seems not to be an option (see here: https://www.oracle.com/technical-resources/articles/java/jf14-date-time.html ).
JabRef tries to be both, ready to use in production environment and be an project to teach about open source programming. So returning to a maybe not-to-far-in-the-future deprecated API seems not possible.

The question is, is there any way to avoid the expensive operations or to cache the pattern etc.

@calixtus
Copy link
Member

calixtus commented Mar 24, 2021

Btw. the tests and checkstyle are failing.

@Ali96kz
Copy link
Contributor Author

Ali96kz commented Mar 25, 2021

I will investigate possible solutions

@calixtus
Copy link
Member

I found some resources to work with the DateTimeFormatter and DateTimeFormatterBuilder, which can probably reduce the cost.
https://www.baeldung.com/java-datetimeformatter

This may be especially interesting:
https://stackoverflow.com/a/45618412

Like something here:

        SIMPLE_DATE_FORMATS = formatStrings.stream()
                                           .map(DateTimeFormatter::ofPattern)
                                           .reduce(new DateTimeFormatterBuilder(),
                                                   DateTimeFormatterBuilder::appendOptional,
                                                   (builder, formatterBuilder) -> builder.append(formatterBuilder.toFormatter()))
                                           .toFormatter(Locale.US);

[...]

        try {
            TemporalAccessor parsedDate = SIMPLE_DATE_FORMATS.parse(dateString, new ParsePosition(0));
            return Optional.of(new Date(parsedDate));
        } catch (DateTimeParseException ignored) {
            return Optional.empty();
        }

Does not yet completely remove the costly stacktrace creation, but provides at least a unified parser for all the patterns.
I read somewhere, that maybe the patters have to be sorted from more complex to simple to provide correct parsing, but I'm not sure if that is the case for the DateTimeFormatterBuilder. That probably would have to be tested.

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Mar 26, 2021
@calixtus
Copy link
Member

Hi @Ali96kz , i took the liberty to push a commit on your branch with a little proposal for a first step towards a solution. You already moved the creation of the patterncompiler out of the parse method, so there should already be an enhancement in speed (even though still a lot of exceptions are probably thrown). However, i hope the new unified DateTimeFormatter, created from the DateTimeFormatterBuilder can enhance the speed a bit more.
The Nullpointer must be thrown, since this is a test convention.

If you don't like my changes, you can always force-push over them.

@Ali96kz
Copy link
Contributor Author

Ali96kz commented Mar 29, 2021

@calixtus I think it solves our problem because the number of exception should not be too much

@calixtus calixtus merged commit 4f213f7 into JabRef:main Mar 29, 2021
@calixtus
Copy link
Member

Thanks for your start @Ali96kz end especially for the part about the early creation of patterns. Let's hope that this speeds up the parsing of Dates.

Siedlerchr added a commit that referenced this pull request Apr 9, 2021
* upstream/main: (25 commits)
  Fix NumberFormatException in BracketedPattern (#7600)
  Update MAINTAINERS (#7601)
  Fix CSL update (#7592)
  Add unit tests for org.jabref.gui classes (#7579)
  Squashed 'buildres/csl/csl-styles/' changes from 30fb68e..e1acabe
  Bump tika-core from 1.25 to 1.26 (#7589)
  Revert "Skip Mac OS build if secret not present (#7591)" (#7594)
  Optimize saving (#7568)
  Skip Mac OS build if secret not present (#7591)
  Rename master to main in dev help
  Update tests-fetchers.yml
  Update refresh-journal-lists.yml
  Update deployment.yml
  Rename master to main in coding readme
  Rename master to main in citation style update
  Rename master to main in gitversion config
  Fix column sort order gets overwritten (#7573)
  Add some hints on test tooling (#7585)
  Add "Update Gradle Wrapper Action" (#7584)
  Bump classgraph from 4.8.102 to 4.8.104 (#7587)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants