Reapply "LSP: Speed up publish diagnostics on project scan."

[dbalek]

Attempt to reapply #7737 reverted by #7982 with additional fixes proposed in #7998.

The aim of this change is to speed up opening projects within the NetBeans Language Server and therefore improve its start up time.
So far, when opening the project, initial scan is performed (similar to regular NetBeans). During this scan all compilation errors (together with their source files and line numbers) are stored in the NB caches. After the scan completes, the Language Server iterates through all the source files with compilation errors (the list of them is obtained from caches) and compiles them again to produce diagnostic that are sent over Language Server Protocol to client - e.g. VSCode that displays them in the Project Problems view.
With the changes in this PR, the Language Server (after the initial scan completes) produces the diagnostic directly from the information stored in caches without the necessity of the second compilation of the source files with compilations errors. To that purpose, the precise positions are added to the information about compilation errors stored in NB chaches during the initial scan. The LSP's Diagnostic requires the range at which the message applies to be specified (not only the line number stored in caches so far).

[mbien]

what is the performance gain of this optimization? What does this solve?

please verify that this doesn't cause the regression again #7981.

there is a rudimentary smoke test #7981 (comment). What testing has been done so far?

[dbalek]

what is the performance gain of this optimization? What does this solve?

More detailed information added to the PR description.

please verify that this doesn't cause the regression again #7981.
there is a rudimentary smoke test #7981 (comment). What testing has been done so far?

I have to admit that I have not run the whole brute force test which opens all NB projects. Instead I ran the short reproducer. No errors appeared in the log file.

[lahodaj]

FWIW, I ran the script that opens all the NB modules. I didn't see nay exception that would seem to be related to this patch. There was one from javac's Analyzer, which may need some investigation. But that is most likely unrelated to this PR. I'll look through the patch in more detail later.

[lahodaj]

Overall, looks good to me. Some comments/proposals inline.

[mbien]

not sure about the spec bump since it is bumped once per release already #8185 - but please squash before merge in any case

[neilcsmith-net]

Yes, please remove the spec bump (I already pointed out this had just been incremented). We handle increments per release already.

[dbalek]

Yes, please remove the spec bump (I already pointed out this had just been incremented). We handle increments per release already.

@lahodaj what do you think? So far we have increased the versions with every change to the module's APIs which was consistent with records in apichanges.xml documents (each record in the apichanges.xml has its own "since" version).

[lahodaj]

Sorry for belated answer. Traditionally, and in the strict API dependency sense, when adding a new API entry point, one needs to increment the module's version, so that other modules can require the new version, so that they can reliably use the new API.

It is true that inside NetBeans itself, we didn't put too much effort into keeping the use sites up-to-date (i.e. module A is using API from module B, but is not explicitly requiring the version of module B where the API was introduced). So, the case is not very strong.

I am not sure what's the advantage of not incrementing the spec version? We presumably still want meaningful @since tags, and at least an apichanges entry, what's the cost that would be saved by not incrementing the specification version?

[neilcsmith-net]

Spec version changes can be a cause of issue during delivery phases. Sigfiles too, although not here, for related reasons.

The first step after delivery branch is increasing every spec version of every module. From a release API point of view, new additions are covered by that already. Of course we want meaningful @since tags. I would question if we have a need for multiple spec version increments between releases? And if there is a reason we still need that, we might rethink the current approach we're using at branch.

[lahodaj]

Not sure I understand the problem. The master already has new specification versions, so unclear how changing the version on master would cause some new problems when merging from delivery?

Overall, nothing will break catastrophically if we coalescence multiple API changes under one version. It is just slightly less precise. OTOH, I am still unclear what is the scarce resource we are trying to conserve here, by coalescing the API changes under the single version. I.e. what is the benefit of only updating the versions for release?

[neilcsmith-net]

Assuming linear progression, generally not an issue. Throw in a need to eg. revert a PR that includes a spec version change through both branches and it can cause some problems to iron out. Isolation is useful. The scarce resource is always time! 😄

The other time consuming thing is of course having to discuss it. We should have one approach here, whichever that is.

[mbien]

i did mention module versioning on the 1-repo-2-schedules thread. Although at that point close to no details were known how the second schedule would actually look or how it would work so a lot of what I wrote are hypotheticals - the problem still applies. One spec version bump per major version is not gong to work anymore most likely since versioning is directly linked to releases. (although the version hash codes help to avoid the worst case scenario: one module with same version built twice containing different content)

[neilcsmith-net]

@mbien we've had basically one repo, two schedules, ever since VSNetBeans had interim releases. We had a discussion about whether we needed extra spec version increments at branching with @MartinBalin a while ago, and it was determined it was not needed by VSNetBeans.

Having a spec version increment across all modules per branch point of either project is not really a problem either, although it is another reason for the necessity of coordinated branch points. Having them in the feature PRs can be, and has been, a problem. So if we don't need them, I'd prefer we didn't have them. If we do, we factor them in. Either way we could do with a consistent approach, and probably discussing on dev@ not here.

For a concrete example of this causing extra work, and rather relevant to this PR! - #8007

[mbien]

we've had basically one repo, two schedules, ever since VSNetBeans had interim releases

yes I know that there already are extra releases - now there would be even more

[neilcsmith-net]

@mbien yes, but the primary point I'm making is that because of that we already had the discussion about whether extra spec increments were necessary to support VSNetBeans' schedule, and it was determined they were not. Unless that point actually needs revision, there's no impact there from having more releases.

[matthiasblaesing]

Spec version changes can be a cause of issue during delivery phases. Sigfiles too, although not here, for related reasons.

I'm somehow missing the problem. The referenced conflict was trivial and can locally by trivially fixed. The git fetch, merge, fix, push cycle ranges somewhere in the low minute range. Yes it is work, but the cumulated time taken to discuss this already dwarfs that.

Bumping spec version on major updates, API changes and/or library updates is an establish practice and was even suggested to do as best practice for library updates. As @lahodaj this is not always strictly handled, but I think it is similar to what was done before Apache NetBeans.

[neilcsmith-net]

Sure, the merge conflict itself can be fixed quite quickly locally, as long as it's done in the way outlined, and never via GitHub's UI. It's a little ugly because we have to ensure delivery can merge cleanly to master and to releaseXX in order for testing to keep working. Anything that involves known merge conflicts going in to delivery generally involves extra coordination, emails and delays in merging things between branches.

I agree, the cumulated time taken here is starting to dwarf the problem, although experience tells me it's not quite as trivial as you make out. Personally, I'd just like to see consistency, rather than half taking one approach and half another.

[lahodaj]

I am not clear what's the status here. I believe this change is not really problematic, besides the specification version change. While I see benefit in continuing to increase the spec version when doing API changes, I can live with updating the versions only once per release.

So, I think we need a decision on whether to drop the spec version change, or continue with this patch as is.

(While reverting API changes might be more tricky, reverting API changes is inevitably tricky. I would not see ease of revert of API changes as a significant argument for process design.)

[neilcsmith-net]

I would prefer having one spec version increment per API release, but not enough to put more time into the conversation on this PR. It's not a blocker. I just don't see the point in doing this as it's not applied consistently, causes minor headaches (not a problem, lots of things do), but primarily doesn't tell you anything useful.

While reverting API changes might be more tricky, reverting API changes is inevitably tricky.

Reverting anything prior to release is fairly simple. Reverting API after a release is generally a no-go. VSNetBeans interim releases are ignored there. The spec version in the previous PR of this told you nothing. That spec version now corresponds to a different API. An API that is tested for breakage via CI.

Each spec version being a snapshot of released API makes sense to me. Anything else is in flux and cannot be relied upon, including currently by VSNetBeans. We have previously reverted features that have been released solely in VSNetBeans. The question of whether spec versions need to be relevant to VSNetBeans, as in API's are fixed at each VSNetBeans branch point, as @mbien alludes to above, then needs to be discussed ... but not on this PR.

[mbien]

PR queue for delivery is empty atm and the release is coming closer so the risk that this is causing conflicts right now is very small. Reading the thread I think the consensus is to get this in as-is and I agree.

I also agree that we have to discuss this somewhere how to proceed with version changes - otherwise this will repeat periodically under PRs. If one spec bump per NB release is still enough or not etc and what problems we are trying to solve with it. To me personally the logic is pretty simple: if there was a spec bump since last release already -> don't bump, simply refer to the specific version in the doc. Having bumps which never appear in maven central is just extra work and might be also confusing for the user who might be looking for artifacts which never existed.