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

Assort fixes to diagnostics and progress notifications #792

Merged
merged 12 commits into from
Jan 14, 2019

Conversation

jvican
Copy link
Contributor

@jvican jvican commented Jan 14, 2019

Changes:

d607a57 (Jorge Vicente Cantero, 87 minutes ago)
Fix test case for #785 and improve reporting if no-op

  • adding a bunch more tests that battle-tests the current changes.

e2c1818 (Jorge Vicente Cantero, 2 days ago)
Improve bsp test infra and add diagnostics test case

Fixes #785

4913130 (Jorge Vicente Cantero, 3 days ago)
Refactor and simplify bsp compile test suite

A first step towards having better tests to check the correct behavior of
all the compilation engine through the lens of BSP.

We could simplify further, but tests already look pretty, so we'll do that
in independent future pull requests if anything.

dacec8d (Jorge Vicente Cantero, 4 days ago)
Change order of logs because it's non-deterministic

3dfedcc (jvican, 4 days ago)
Clear previous problems as early as possible

BSP requires that bloop clears problems that were present in previous
compiler iterations manually. Previously, Bloop would meet this contract by
clearing problems from previous compiler runs at the end of a successful
incremental cycle. For every incremental cycle, bloop would go through the
problems and clear those that were affected by changes occurred during that
cycle. You can see more information in this pull request:
#782

However, this approach was slower than desired and required that we
successfully finished an incremental compiler cycle before starting to
clean some diagnostics in the recompiled files.

Instead, this pull request takes a different approach to this problem. We
clear diagnostics as soon as we know they are safe to be cleared. For
example, if in a previous compiler run typer emitted an error, we have
bloop record that a diagnostic happened during that phase and, in
subsequent compiler runs, we clear the diagnostic as soon as we've passed
typer for the source file that contained the error.

That is, we clear diagnostics right after the phase that generated them
succeeded.

If a diagnostic could not be mapped to a phase, we fallback to the previous
behavior: we will clear it in reportRemainingProblems right after every
incremental cycle.

4d2f9f3 (Jorge Vicente Cantero, 5 days ago)
Report progress notifications every 5% increments

Fixes #788

a50095e (jvican, 5 days ago)
Fix name in cancelled variable

c8552e3 (jvican, 5 days ago)
Log rich compilation msg in bsp compile task start

Imitates the same logging behavior when a compilation is triggered in the
CLI Compiling foo (1 Scala source).

037759c (jvican, 5 days ago)
Report start and end of compilation per incremental cycle

Previously, the start and end of the compilation would be reported once per
build target compilation. With this change, bloop reports the start and end
of compilation at the granularity of the incremental cycle.

This helps:

  1. Reporting precise progress notifications to clients (e.g. Metals), as
    we know all the total units upfront. 2. Exposes information about how
    many incremental cycles are used to
    compile a project. The more transparent we are with this information,
    the better.

df8a8fe (jvican, 5 days ago)
Fix order of bsp task progress params

c951b7f (jvican, 5 days ago)
Send progress notifications after the phase runs

jvican added 11 commits January 10, 2019 20:22
Previously, the start and end of the compilation would be reported once
per build target compilation. With this change, bloop reports the start
and end of compilation at the granularity of the incremental cycle.

This helps:

1. Reporting precise progress notifications to clients (e.g. Metals), as
   we know all the total units upfront.
2. Exposes information about how many incremental cycles are used to
   compile a project. The more transparent we are with this information,
   the better.
Imitates the same logging behavior when a compilation is triggered in
the CLI `Compiling foo (1 Scala source)`.
BSP requires that bloop clears problems that were present in previous
compiler iterations manually. Previously, Bloop would meet this contract
by clearing problems from previous compiler runs at the end of a
successful incremental cycle. For every incremental cycle, bloop would
go through the problems and clear those that were affected by changes
occurred during that cycle. You can see more information in this pull
request: #782

However, this approach was slower than desired and required that we
successfully finished an incremental compiler cycle before *starting* to
clean some diagnostics in the recompiled files.

Instead, this pull request takes a different approach to this problem.
We clear diagnostics as soon as we know they are safe to be cleared. For
example, if in a previous compiler run typer emitted an error, we have
bloop record that a diagnostic happened during that phase and, in
subsequent compiler runs, we clear the diagnostic as soon as we've
passed typer for the source file that contained the error.

That is, we clear diagnostics right after the phase that generated them
succeeded.

If a diagnostic could not be mapped to a phase, we fallback to the
previous behavior: we will clear it in `reportRemainingProblems` right
after every incremental cycle.
A first step towards having better tests to check the correct behavior
of all the compilation engine through the lens of BSP.

We could simplify further, but tests already look pretty, so we'll do
that in independent future pull requests if anything.
+ adding a bunch more tests that battle-tests the current changes.
@jvican jvican force-pushed the topic/improve-progress-notification branch from e3c0236 to 73e426e Compare January 14, 2019 14:27
@olafurpg
Copy link
Contributor

Great job! The Metals test suite passes with these changes and I'm no longer able to reproduce #785. I can also confirm that diagnostics are cleared much faster now 👍

@jvican jvican added bug A defect or misbehaviour. build server Any issue or pull request that has to do with hot compilers or BSP. labels Jan 14, 2019
@jvican jvican merged commit 1191afd into master Jan 14, 2019
@jvican jvican deleted the topic/improve-progress-notification branch January 14, 2019 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect or misbehaviour. build server Any issue or pull request that has to do with hot compilers or BSP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants