NETBEANS-2408 use System.currentTimeMillis()

[bd2019us]

Hello,
new Date() is just a thin wrapper around System.currentTimeMillis(). Using System.currentTimeMillis() can help speed up the system.

[lkishalmi]

First, nor your github neither your JIRA account is identifiable. Please fix those before sending a PR.
Second, do not change the code for the sake of the change. You need to prove that the actual part you are fixing is actually cause bad performance. I have serious doubt, that the changed code is actually called more than a few dozen't times, especially as even in the age of introduction Java EE AppClient was not really popular, so If you happen to write a Java EE module with AppClient support and you might get some notifications on project errors, then this method would be invoked.
If you have serious reasons to change new Date().getTime() to System.currentTimeMillis(), then you need to prove it and probably replace all occurrences.
In this form NETBEANS-2408 is not a bug and this PR shall be closed without merge.

[matthiasblaesing]

Without having looked to deeply into the functionality the method showErrorMessage indicates, that we are already in the slow path, so performance is not a too big concern. There are also other callers in netbeans, that would raise the question why these were not modified.

The concern about the missing real identification was already raised in #1182 and needs to be addressed.

[lkishalmi]

I have the feeling this guy just would like to gather some Open Source footprint for whatever reason, by 'traveling around' with the same kind of "harmless" stuff in a few dozen Apache (maybe other) repositories. He joined to GitHub two months ago and already had 140 "contributions" to 58 repositories. He usually offers File.mkdir() -> File.mkdirs() or new Date().getTime() -> System.currentTimeMillis() changes.

Lets give @bd2019us a week to speak up for himself. Otherwise we shall close the PR-s and the JIRA tickets.

[bd2019us]

Hi @matthiasblaesing and @lkishalmi, sorry for our late reply and the trouble to you due to our identification issue.

We are a research group majoring in Software Engineering from a university in America, and we are working on a project related to bug detection in open-source projects. However, because of the anonymous policy of paper publication, we did not disclose our real identification. We are really sorry for the inconvenience. Definitely, if you are still not convinced, we are also happy to provide our detailed information by some way. In all, thanks for your understanding and support.

As for the code changes, all PRs were carefully written and examined by experienced developers in our group. The target of our research is to improve the quality of open-source projects. Though the improvement of some code changes is not very high, we still think it can help in some degree. Therefore, we appreciate for your kind consideration for the PRs.

Looking forward to your reply. Thanks.

[matthiasblaesing]

Sorry - I don't buy that argumentation. I'm not a researcher, but I doubt any researcher would quote a work or use foreign research without knowing the author (even if just to be able to quote him/her). You are responsible for your work and thus should stand up to it with your name.

In this case I wonder for example, why line 430 was not changed. You also did not address the question why the change in this slow path method is a good idea, but not other cases in the netbeans code base.

[bd2019us]

Hello!
We are sorry that we missed other cases like in line 430. The reason is that all the changed lines are firstly suggested by an automatic bug detection tool and then we manually checked each of them. In this process, we may miss some of them. Thanks for your kind reminding. If possible, we can finish the PR with careful check of all locations that may be relevant.

Following example shows that using currentTimeMillis is faster than new Date().getTime() when it is called many times.

Also, to be convinced, we have updated the personal information in the profile.
Sorry for the inconvenience again and thanks for your understanding!

[lleming]

There is a special performance utility http://openjdk.java.net/projects/code-tools/jmh/. Anyway, this test is abstract it just shows the obvious difference due to the nature of current circumstances. The reality is different. The current line will not be called thousands of times, every second. Better to improve code readability, using java date time api.

[Chris2011]

I think the code is still readable. It doesn't matter whether I use new Date().getTime(); or System.getCurrentTimeMillis. And for me, the System... is more readable thatn the others. getCurrentTimeMillis give us exact what it is there for. My 2c.

[lkishalmi]

Just being curious is this the only one new Date().getTime() in the entire codebase? If this PR fixes all occurrences (even that this is the only one), then from me I can let it go, however let it be clear in the JIRA issue that we are doing this for readability not for performance.

[bd2019us]

Just being curious is this the only one new Date().getTime() in the entire codebase? If this PR fixes all occurrences (even that this is the only one), then from me I can let it go, however let it be clear in the JIRA issue that we are doing this for readability not for performance.

Hello, @lkishalmi. I just updated all locations of new Date().getTime(). Please check if they are good for PR fixes. Thanks!

[lkishalmi]

This is Ok with me now, I changed my viewpoint to natural: +0. If anyone else have other feelings about this change I respect that.

[wadechandler]

I agree may be a relatively small bang for the buck change, but not worth fussing over. Xia (the op) has now updated the user information; which I agree "anonymous" contributors is not something we want and is not in line with "The Apache Way", but satisfied with the current state. The PR certainly reduces creating arbitrary objects. Thanks for the contribution.

[wadechandler]

@matthiasblaesing are you satisfied with the changes now?

[matthiasblaesing]

I will not stand in the way of the merge, but I won't do it. IMHO the change is unnecessary. In SQLHistoryEntry.java the change reduces readability, as before java.util.Date was used for all date cases, now to different codepaths are taken.

[matthiasblaesing]

As no-one came up to merge this in one month, I'm closing this.