#6057 Improve startup time #7486
Conversation
|
I like the idea, but please check the org.jabref.gui.util.comparator.NumericFieldComparatorTest |
|
Thanks, that's indeed a nice improvement. However, regex are also pretty expensive, so there is still room for improvement. For example, https://stackoverflow.com/questions/11875424/java-regular-expression-offers-any-performance-benefit discusses a neat way using character-wise checks. There are some issues with this approach e.g. with negative or decimal numbers, but that's not really something we care about in the main table, so we can ignore them. Another huge improvement would be to not use the numeric comparator for all fields by default. For example, it doesn't make any sense to try comparing authors by converting them first to numbers. I think, only the year columns needs such a treatment...and custom columns for which we don't know the format #6349. |
|
@tobiasdiez I changed the number validation mechanism. I don't know how to implement the second suggestion because there is no way to get data type from |
tobiasdiez
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. Two minor remarks from my side.
Concerning the installation of the compartor in the main table, you have the field available at
.Thus you could add a check at if the field is a
UnknownField or if it is numeric:
tobiasdiez
left a comment
There was a problem hiding this comment.
The checkstyle fails; apart from this is good to go from my side.
|
@k3KAW8Pnf7mkmdSMPHz27 Are you a second reviewer? |
|
@Ali96kz unless someone else is taking a look at it earlier, I should be able to review it in a couple of hours. |
There was a problem hiding this comment.
It looks good to me! There is only one change I truly require (line 51 in NumericFieldComparator) and the rest are perhaps nitpicking.
Other than that, good changes. I am looking forward to the performance improvement 😍
(Sorry about the slightly later than promised review, I were less familiar with OrFields than I expected)
| } catch (NumberFormatException ignore) { | ||
| // do nothing | ||
| // If we arrive at this stage then both values are actually numeric ! | ||
| return valInt1 - valInt2; |
There was a problem hiding this comment.
I know this isn't your code, but I'd rather see valInt1.compareTo(valInt2) here (even if it should not overflow with reasonable values). I believe it will help with avoiding auto-(un)boxing as well.
There was a problem hiding this comment.
It will break a test
There was a problem hiding this comment.
For compareTwoNumericInputs I'd go either with 1 or changing it along the lines of https://junit.org/junit5/docs/snapshot/user-guide/index.html#writing-tests-test-interfaces-and-default-methods
e.g.,
assertTrue(comparator.compare("4", "2") > 0);
Siedlerchr
left a comment
There was a problem hiding this comment.
thanks for tackling this issue!
#6057 Check string is number with regex instead of throwing exception. It will improve start time because when I have 3000+ entries it throws about ~170.000 NumberFormatException
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)