[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6b85032c95be: [clangd] Update TUStatus api to accommodate
preamble thread (authored by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include
+#include
#include
#include
@@ -40,13 +41,14 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-MATCHER_P2(TUState, State, ActionName, "") {
- if (arg.Action.S != State) {
-*result_listener << "state is " << arg.Action.S;
+MATCHER_P2(TUState, PreambleActivity, ASTActivity, "") {
+ if (arg.PreambleActivity != PreambleActivity) {
+*result_listener << "preamblestate is "
+ << static_cast(arg.PreambleActivity);
return false;
}
- if (arg.Action.Name != ActionName) {
-*result_listener << "name is " << arg.Action.Name;
+ if (arg.ASTActivity.K != ASTActivity) {
+*result_listener << "aststate is " << arg.ASTActivity.K;
return false;
}
return true;
@@ -732,14 +734,13 @@
// Update the source contents, which should trigger an initial build with
// the header file missing.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(
-Diags,
-ElementsAre(
-Field(::Message, "'foo.h' file not found"),
-Field(::Message, "use of undeclared identifier 'a'")));
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+EXPECT_THAT(Diags,
+ElementsAre(Field(::Message, "'foo.h' file not found"),
+Field(::Message,
+ "use of undeclared identifier 'a'")));
+ });
// Add the header file. We need to recreate the inputs since we changed a
// file from underneath the test FS.
@@ -749,18 +750,17 @@
// The addition of the missing header file shouldn't trigger a rebuild since
// we don't track missing files.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-ADD_FAILURE() << "Did not expect diagnostics for missing header update";
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+ADD_FAILURE() << "Did not expect diagnostics for missing header update";
+ });
// Forcing the reload should should cause a rebuild which no longer has any
// errors.
Inputs.ForceRebuild = true;
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(Diags, IsEmpty());
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes,
+ [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
@@ -848,14 +848,21 @@
EXPECT_THAT(CaptureTUStatus.allStatus(),
ElementsAre(
- // Statuses of "Update" action.
- TUState(TUAction::RunningAction, "Update (1)"),
- TUState(TUAction::BuildingPreamble, "Update (1)"),
- TUState(TUAction::BuildingFile, "Update (1)"),
-
- // Statuses of "Definitions" action
- TUState(TUAction::RunningAction, "Definitions"),
- TUState(TUAction::Idle, /*No action*/ "")));
+ // Everything starts with ASTWorker starting to execute an
+ // update
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // We build the preamble
+ TUState(PreambleAction::Building, ASTAction::RunningAction),
+ // Preamble worker goes idle
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // We start building the ast
+ TUState(PreambleAction::Idle, ASTAction::Building),
+ // Built finished succesffully
+ TUState(PreambleAction::Idle, ASTAction::Building),
+ // Rnning go to def
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // both workers go idle
+ TUState(PreambleAction::Idle, ASTAction::Idle)));
}
TEST_F(TUSchedulerTests, CommandLineErrors) {
@@ -868,8 +875,7 @@
TUScheduler S(CDB, optsForTest(),
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
kadircet updated this revision to Diff 254519.
kadircet added a comment.
- Rebase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include
+#include
#include
#include
@@ -40,13 +41,14 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-MATCHER_P2(TUState, State, ActionName, "") {
- if (arg.Action.S != State) {
-*result_listener << "state is " << arg.Action.S;
+MATCHER_P2(TUState, PreambleActivity, ASTActivity, "") {
+ if (arg.PreambleActivity != PreambleActivity) {
+*result_listener << "preamblestate is "
+ << static_cast(arg.PreambleActivity);
return false;
}
- if (arg.Action.Name != ActionName) {
-*result_listener << "name is " << arg.Action.Name;
+ if (arg.ASTActivity.K != ASTActivity) {
+*result_listener << "aststate is " << arg.ASTActivity.K;
return false;
}
return true;
@@ -732,14 +734,13 @@
// Update the source contents, which should trigger an initial build with
// the header file missing.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(
-Diags,
-ElementsAre(
-Field(::Message, "'foo.h' file not found"),
-Field(::Message, "use of undeclared identifier 'a'")));
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+EXPECT_THAT(Diags,
+ElementsAre(Field(::Message, "'foo.h' file not found"),
+Field(::Message,
+ "use of undeclared identifier 'a'")));
+ });
// Add the header file. We need to recreate the inputs since we changed a
// file from underneath the test FS.
@@ -749,18 +750,17 @@
// The addition of the missing header file shouldn't trigger a rebuild since
// we don't track missing files.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-ADD_FAILURE() << "Did not expect diagnostics for missing header update";
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+ADD_FAILURE() << "Did not expect diagnostics for missing header update";
+ });
// Forcing the reload should should cause a rebuild which no longer has any
// errors.
Inputs.ForceRebuild = true;
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(Diags, IsEmpty());
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes,
+ [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
@@ -848,14 +848,21 @@
EXPECT_THAT(CaptureTUStatus.allStatus(),
ElementsAre(
- // Statuses of "Update" action.
- TUState(TUAction::RunningAction, "Update (1)"),
- TUState(TUAction::BuildingPreamble, "Update (1)"),
- TUState(TUAction::BuildingFile, "Update (1)"),
-
- // Statuses of "Definitions" action
- TUState(TUAction::RunningAction, "Definitions"),
- TUState(TUAction::Idle, /*No action*/ "")));
+ // Everything starts with ASTWorker starting to execute an
+ // update
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // We build the preamble
+ TUState(PreambleAction::Building, ASTAction::RunningAction),
+ // Preamble worker goes idle
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // We start building the ast
+ TUState(PreambleAction::Idle, ASTAction::Building),
+ // Built finished succesffully
+ TUState(PreambleAction::Idle, ASTAction::Building),
+ // Rnning go to def
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // both workers go idle
+ TUState(PreambleAction::Idle, ASTAction::Idle)));
}
TEST_F(TUSchedulerTests, CommandLineErrors) {
@@ -868,8 +875,7 @@
TUScheduler S(CDB, optsForTest(), captureDiags());
std::vector Diagnostics;
updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
-
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
build(std::move(*CurrentReq));
+ bool IsEmpty = false;
+ {
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > Seems clearer to do this immediately before blocking?
> > >
> > > at the top:
> > >
> > > ```
> > > if (!NextReq) {
> > > Lock.unlock();
> > > StatusManager.setPreambleAction(Idle);
> > > Lock.lock();
> > > ReqCV.wait(NextReq || Done);
> > > }
> > > if (Done)
> > > break;
> > > ```
> > i agree, but I wanted to keep the current semantics. we only start emitting
> > tustatuses after thread starts executing the first request.
> >
> > happy to change *both* preamblethread and astworker to emit before blocking
> > though. wdyt?
> I think the difference is moot - we never create either the AST or preamble
> worker until there's something to do.
>
> The code around scheduling/sleeping in the AST worker thread is way more
> complicated, and I'm not confident moving the status broadcast to the top of
> the loop would be clearer there.
>
> Up to you: if you think both are clearer, move both. If you think the
> preamble is clearer at the top and AST worker at the bottom, then you can
> choose between consistency and clarity :-)
as discussed offline, this might make tests flaky due to a possible race
between this thread and the thread issuing the write. so leaving it as it is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
kadircet updated this revision to Diff 252248.
kadircet marked 14 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include
+#include
#include
#include
@@ -40,13 +41,14 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-MATCHER_P2(TUState, State, ActionName, "") {
- if (arg.Action.S != State) {
-*result_listener << "state is " << arg.Action.S;
+MATCHER_P2(TUState, PreambleActivity, ASTActivity, "") {
+ if (arg.PreambleActivity != PreambleActivity) {
+*result_listener << "preamblestate is "
+ << static_cast(arg.PreambleActivity);
return false;
}
- if (arg.Action.Name != ActionName) {
-*result_listener << "name is " << arg.Action.Name;
+ if (arg.ASTActivity.K != ASTActivity) {
+*result_listener << "aststate is " << arg.ASTActivity.K;
return false;
}
return true;
@@ -732,14 +734,13 @@
// Update the source contents, which should trigger an initial build with
// the header file missing.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(
-Diags,
-ElementsAre(
-Field(::Message, "'foo.h' file not found"),
-Field(::Message, "use of undeclared identifier 'a'")));
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+EXPECT_THAT(Diags,
+ElementsAre(Field(::Message, "'foo.h' file not found"),
+Field(::Message,
+ "use of undeclared identifier 'a'")));
+ });
// Add the header file. We need to recreate the inputs since we changed a
// file from underneath the test FS.
@@ -749,18 +750,17 @@
// The addition of the missing header file shouldn't trigger a rebuild since
// we don't track missing files.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-ADD_FAILURE() << "Did not expect diagnostics for missing header update";
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+ADD_FAILURE() << "Did not expect diagnostics for missing header update";
+ });
// Forcing the reload should should cause a rebuild which no longer has any
// errors.
Inputs.ForceRebuild = true;
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(Diags, IsEmpty());
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes,
+ [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
@@ -848,14 +848,21 @@
EXPECT_THAT(CaptureTUStatus.allStatus(),
ElementsAre(
- // Statuses of "Update" action.
- TUState(TUAction::RunningAction, "Update (1)"),
- TUState(TUAction::BuildingPreamble, "Update (1)"),
- TUState(TUAction::BuildingFile, "Update (1)"),
-
- // Statuses of "Definitions" action
- TUState(TUAction::RunningAction, "Definitions"),
- TUState(TUAction::Idle, /*No action*/ "")));
+ // Everything starts with ASTWorker starting to execute an
+ // update
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // We build the preamble
+ TUState(PreambleAction::Building, ASTAction::RunningAction),
+ // Preamble worker goes idle
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // We start building the ast
+ TUState(PreambleAction::Idle, ASTAction::Building),
+ // Built finished succesffully
+ TUState(PreambleAction::Idle, ASTAction::Building),
+ // Rnning go to def
+ TUState(PreambleAction::Idle, ASTAction::RunningAction),
+ // both workers go idle
+ TUState(PreambleAction::Idle, ASTAction::Idle)));
}
TEST_F(TUSchedulerTests, CommandLineErrors) {
@@ -868,8 +875,7 @@
TUScheduler S(CDB, optsForTest(), captureDiags());
std::vector Diagnostics;
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
build(std::move(*CurrentReq));
+ bool IsEmpty = false;
+ {
kadircet wrote:
> sammccall wrote:
> > Seems clearer to do this immediately before blocking?
> >
> > at the top:
> >
> > ```
> > if (!NextReq) {
> > Lock.unlock();
> > StatusManager.setPreambleAction(Idle);
> > Lock.lock();
> > ReqCV.wait(NextReq || Done);
> > }
> > if (Done)
> > break;
> > ```
> i agree, but I wanted to keep the current semantics. we only start emitting
> tustatuses after thread starts executing the first request.
>
> happy to change *both* preamblethread and astworker to emit before blocking
> though. wdyt?
I think the difference is moot - we never create either the AST or preamble
worker until there's something to do.
The code around scheduling/sleeping in the AST worker thread is way more
complicated, and I'm not confident moving the status broadcast to the top of
the loop would be clearer there.
Up to you: if you think both are clearer, move both. If you think the preamble
is clearer at the top and AST worker at the bottom, then you can choose between
consistency and clarity :-)
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:357
+
+ SynchronizedTUStatus
};
StatusManager -> Status
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:490
+ SynchronizedTUStatus StatusManager;
PreambleThread PW;
StatusManager -> Status
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:698
+ // buildAST fails.
+ if (!(*AST)) {
+StatusManager.update(
(might be clearer to call once and make the logic conditional)
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1053
+ llvm::SmallVector Result;
+ if (PA == PreambleAction::Building)
+Result.push_back("parsing includes");
nit: write this as a switch for consistency plus the covered warning?
Comment at: clang-tools-extra/clangd/TUScheduler.h:122
- TUAction Action;
- BuildDetails Details;
+ /// Stores the status of preamble thread.
+ PreambleAction PreambleState = PreambleAction::Idle;
comment just repeats the declaration, drop it?
Comment at: clang-tools-extra/clangd/TUScheduler.h:123
+ /// Stores the status of preamble thread.
+ PreambleAction PreambleState = PreambleAction::Idle;
+ ASTAction ASTState;
I think these variables are still best described at a high level actions, not
states. (Well, the state of the thread is that it is running this action, but
that's a little roundabout. TUAction::State probably should have been called
Kind instead).
Unfortunate that we need both a type name and a variable name though.
`ASTAction ASTActivity; PreambleAction PreambleActivity;`?
Comment at: clang-tools-extra/clangd/TUScheduler.h:126
+ /// Stores status of the last build for the translation unit.
+ BuildStatus BS = None;
};
If keeping the enum, this variable needs a real name.
I'd suggest `LastBuild` and dropping the comment.
Comment at: clang-tools-extra/clangd/TUScheduler.h:104
struct TUStatus {
- struct BuildDetails {
-/// Indicates whether clang failed to build the TU.
-bool BuildFailed = false;
-/// Indicates whether we reused the prebuilt AST.
-bool ReuseAST = false;
+ enum BuildStatus {
+/// Haven't run a built yet
kadircet wrote:
> sammccall wrote:
> > What's the purpose of this change? It seems to make the build details
> > harder to maintain/extend.
> >
> > (In particular, say we were going to add a "fallback command was used" bit,
> > where would it go after this change?)
> this was to narrow down the interface. Currently builddetails is only being
> used to report buildstatus, and those cases are mutually exclusive(a build
> can't be both a reuse and failure). Therefore i wanted to make that more
> explicit.
>
> if we decide to add more details, I would say we should rather get
> BuildDetails struct back, with a Status enum and a couple more state to
> signal any other information.
OK, I understand but don't really agree - the fact that these are exclusive is
a coincidence that we don't need to model APIs around. This doesn't seem
related to the rest of this patch, either.
I think we're going around in circles on this though, will leave this up to you.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:161
+/// update.
+class TUStatusManager {
+public:
sammccall wrote:
> nit: "manager" doesn't really explain what this is, and it's used both as the
> class name and the main description in the comment.
>
> Maybe TUStatusTracker or something - "Threadsafe holder for TUStatus..."
NAMNG !!! :(((
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:198
+
+ mutable std::mutex StatusMu;
+ TUStatus Status;
sammccall wrote:
> why mutable with no const functions?
I had a readable version at some point, just leftover from those days :P
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
build(std::move(*CurrentReq));
+ bool IsEmpty = false;
+ {
sammccall wrote:
> Seems clearer to do this immediately before blocking?
>
> at the top:
>
> ```
> if (!NextReq) {
> Lock.unlock();
> StatusManager.setPreambleAction(Idle);
> Lock.lock();
> ReqCV.wait(NextReq || Done);
> }
> if (Done)
> break;
> ```
i agree, but I wanted to keep the current semantics. we only start emitting
tustatuses after thread starts executing the first request.
happy to change *both* preamblethread and astworker to emit before blocking
though. wdyt?
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:285
+StatusManager.setPreambleAction({TUAction::Idle, /*Name=*/""});
+ BuiltFirst.notify();
+ ReqCV.notify_all();
sammccall wrote:
> why this change?
this needs to happen after resetting `CurrentReq` and previously this was part
of `build`. It makes more sense to hanlde this resetting as part of the worker
thread, rather than as part of the preamble build logic.
but I suppose it doesn't belong into this patch. moving it to the
https://reviews.llvm.org/D76125
Comment at: clang-tools-extra/clangd/TUScheduler.h:104
struct TUStatus {
- struct BuildDetails {
-/// Indicates whether clang failed to build the TU.
-bool BuildFailed = false;
-/// Indicates whether we reused the prebuilt AST.
-bool ReuseAST = false;
+ enum BuildStatus {
+/// Haven't run a built yet
sammccall wrote:
> What's the purpose of this change? It seems to make the build details harder
> to maintain/extend.
>
> (In particular, say we were going to add a "fallback command was used" bit,
> where would it go after this change?)
this was to narrow down the interface. Currently builddetails is only being
used to report buildstatus, and those cases are mutually exclusive(a build
can't be both a reuse and failure). Therefore i wanted to make that more
explicit.
if we decide to add more details, I would say we should rather get BuildDetails
struct back, with a Status enum and a couple more state to signal any other
information.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
kadircet updated this revision to Diff 251022.
kadircet marked 8 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include
+#include
#include
#include
@@ -40,13 +41,18 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-MATCHER_P2(TUState, State, ActionName, "") {
- if (arg.Action.S != State) {
-*result_listener << "state is " << arg.Action.S;
+MATCHER_P3(TUState, PreambleState, ASTState, BS, "") {
+ if (arg.PreambleState != PreambleState) {
+*result_listener << "preamblestate is "
+ << static_cast(arg.PreambleState);
return false;
}
- if (arg.Action.Name != ActionName) {
-*result_listener << "name is " << arg.Action.Name;
+ if (arg.ASTState.S != ASTState) {
+*result_listener << "aststate is " << arg.ASTState.S;
+return false;
+ }
+ if (arg.BS != BS) {
+*result_listener << "builddetail is " << arg.BS;
return false;
}
return true;
@@ -732,14 +738,13 @@
// Update the source contents, which should trigger an initial build with
// the header file missing.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(
-Diags,
-ElementsAre(
-Field(::Message, "'foo.h' file not found"),
-Field(::Message, "use of undeclared identifier 'a'")));
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+EXPECT_THAT(Diags,
+ElementsAre(Field(::Message, "'foo.h' file not found"),
+Field(::Message,
+ "use of undeclared identifier 'a'")));
+ });
// Add the header file. We need to recreate the inputs since we changed a
// file from underneath the test FS.
@@ -749,18 +754,17 @@
// The addition of the missing header file shouldn't trigger a rebuild since
// we don't track missing files.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-ADD_FAILURE() << "Did not expect diagnostics for missing header update";
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+ADD_FAILURE() << "Did not expect diagnostics for missing header update";
+ });
// Forcing the reload should should cause a rebuild which no longer has any
// errors.
Inputs.ForceRebuild = true;
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(Diags, IsEmpty());
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes,
+ [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
@@ -846,16 +850,28 @@
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(CaptureTUStatus.allStatus(),
- ElementsAre(
- // Statuses of "Update" action.
- TUState(TUAction::RunningAction, "Update (1)"),
- TUState(TUAction::BuildingPreamble, "Update (1)"),
- TUState(TUAction::BuildingFile, "Update (1)"),
-
- // Statuses of "Definitions" action
- TUState(TUAction::RunningAction, "Definitions"),
- TUState(TUAction::Idle, /*No action*/ "")));
+ EXPECT_THAT(
+ CaptureTUStatus.allStatus(),
+ ElementsAre(
+ // Everything starts with ASTWorker starting to execute an
+ // update
+ TUState(PreambleAction::Idle, ASTAction::RunningAction,
+ TUStatus::None),
+ // We build the preamble
+ TUState(PreambleAction::Building, ASTAction::RunningAction,
+ TUStatus::None),
+ // Preamble worker goes idle
+ TUState(PreambleAction::Idle, ASTAction::RunningAction,
+ TUStatus::None),
+ // We start building the ast
+ TUState(PreambleAction::Idle, ASTAction::Building, TUStatus::None),
+ // Built finished succesffully
+ TUState(PreambleAction::Idle, ASTAction::Building, TUStatus::Built),
+ // Rnning go to def
+ TUState(PreambleAction::Idle, ASTAction::RunningAction,
+ TUStatus::Built),
+
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:161
+/// update.
+class TUStatusManager {
+public:
nit: "manager" doesn't really explain what this is, and it's used both as the
class name and the main description in the comment.
Maybe TUStatusTracker or something - "Threadsafe holder for TUStatus..."
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:166
+
+ void setPreambleAction(TUAction A) {
+std::lock_guard Lock(StatusMu);
you could consider a slightly more general version:
```
StatusMgr.update([&](TUStatus ) {
S.PreambleAction = Idle;
});
```
it's a bit more wordy at the callsite but it makes the correspondence much more
direct.
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:198
+
+ mutable std::mutex StatusMu;
+ TUStatus Status;
why mutable with no const functions?
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
build(std::move(*CurrentReq));
+ bool IsEmpty = false;
+ {
Seems clearer to do this immediately before blocking?
at the top:
```
if (!NextReq) {
Lock.unlock();
StatusManager.setPreambleAction(Idle);
Lock.lock();
ReqCV.wait(NextReq || Done);
}
if (Done)
break;
```
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:285
+StatusManager.setPreambleAction({TUAction::Idle, /*Name=*/""});
+ BuiltFirst.notify();
+ ReqCV.notify_all();
why this change?
Comment at: clang-tools-extra/clangd/TUScheduler.h:104
struct TUStatus {
- struct BuildDetails {
-/// Indicates whether clang failed to build the TU.
-bool BuildFailed = false;
-/// Indicates whether we reused the prebuilt AST.
-bool ReuseAST = false;
+ enum BuildStatus {
+/// Haven't run a built yet
What's the purpose of this change? It seems to make the build details harder to
maintain/extend.
(In particular, say we were going to add a "fallback command was used" bit,
where would it go after this change?)
Comment at: clang-tools-extra/clangd/TUScheduler.h:117
- TUAction Action;
- BuildDetails Details;
+ TUAction PreambleAction;
+ TUAction ASTAction;
why TUAction rather than a PreambleAction enum { Building, Idle }?
It seems Name will never be used, RunningAction/Queued are not possible etc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76304/new/
https://reviews.llvm.org/D76304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread
kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
javed.absar, ilya-biryukov.
Herald added a project: clang.
TUStatus api had a single thread in mind. This introudces a section
action to represent state of the preamble thread. In the file status extension,
we keep old behavior almost the same. We only prepend current task with a
`parsing includes` if preamble thread is working. We omit the idle thread in the
output unless both threads are idle.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D76304
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -40,13 +40,17 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-MATCHER_P2(TUState, State, ActionName, "") {
- if (arg.Action.S != State) {
-*result_listener << "state is " << arg.Action.S;
+MATCHER_P3(TUState, PreambleState, ASTState, BS, "") {
+ if (arg.PreambleAction.S != PreambleState) {
+*result_listener << "preamblestate is " << arg.PreambleAction.S;
return false;
}
- if (arg.Action.Name != ActionName) {
-*result_listener << "name is " << arg.Action.Name;
+ if (arg.ASTAction.S != ASTState) {
+*result_listener << "aststate is " << arg.ASTAction.S;
+return false;
+ }
+ if (arg.BS != BS) {
+*result_listener << "builddetail is " << arg.BS;
return false;
}
return true;
@@ -732,14 +736,13 @@
// Update the source contents, which should trigger an initial build with
// the header file missing.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(
-Diags,
-ElementsAre(
-Field(::Message, "'foo.h' file not found"),
-Field(::Message, "use of undeclared identifier 'a'")));
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+EXPECT_THAT(Diags,
+ElementsAre(Field(::Message, "'foo.h' file not found"),
+Field(::Message,
+ "use of undeclared identifier 'a'")));
+ });
// Add the header file. We need to recreate the inputs since we changed a
// file from underneath the test FS.
@@ -749,18 +752,17 @@
// The addition of the missing header file shouldn't trigger a rebuild since
// we don't track missing files.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-ADD_FAILURE() << "Did not expect diagnostics for missing header update";
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+ADD_FAILURE() << "Did not expect diagnostics for missing header update";
+ });
// Forcing the reload should should cause a rebuild which no longer has any
// errors.
Inputs.ForceRebuild = true;
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [](std::vector Diags) {
-EXPECT_THAT(Diags, IsEmpty());
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Yes,
+ [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
@@ -846,16 +848,24 @@
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(CaptureTUStatus.allStatus(),
- ElementsAre(
- // Statuses of "Update" action.
- TUState(TUAction::RunningAction, "Update (1)"),
- TUState(TUAction::BuildingPreamble, "Update (1)"),
- TUState(TUAction::BuildingFile, "Update (1)"),
-
- // Statuses of "Definitions" action
- TUState(TUAction::RunningAction, "Definitions"),
- TUState(TUAction::Idle, /*No action*/ "")));
+ EXPECT_THAT(
+ CaptureTUStatus.allStatus(),
+ ElementsAre(
+ // Everything starts with ASTWorker starting to execute an
+ // update
+ TUState(TUAction::Idle, TUAction::RunningAction, TUStatus::None),
+ // We build the preamble
+ TUState(TUAction::Building, TUAction::RunningAction, TUStatus::None),
+ // Preamble worker goes idle
+ TUState(TUAction::Idle, TUAction::RunningAction, TUStatus::None),
+ // We start building the ast
+ TUState(TUAction::Idle, TUAction::Building, TUStatus::None),
+ // Built finished succesffully
+ TUState(TUAction::Idle, TUAction::Building, TUStatus::Built),
+ // Rnning go to
