DmitryPolukhin added inline comments.
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:694
Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
- return Result;
+ CapturedCtx.emplace(CapturedInfo.takeLife());
+ return std::make_pair(Result, CapturedCtx);
----------------
kuganv wrote:
> kadircet wrote:
> > kuganv wrote:
> > > kadircet wrote:
> > > > what about just keeping the callback (with a different signature) and
> > > > calling it here? e.g.:
> > > > ```
> > > > PreambleCallback(CapturedInfo.takeLife());
> > > > ```
> > > >
> > > > that way we don't need to change the return type and also control if we
> > > > received a valid object on every call site (since callback will only be
> > > > invoked on success)
> > > > what about just keeping the callback (with a different signature) and
> > > > calling it here? e.g.:
> > > > ```
> > > > PreambleCallback(CapturedInfo.takeLife());
> > > > ```
> > > >
> > > > that way we don't need to change the return type and also control if we
> > > > received a valid object on every call site (since callback will only be
> > > > invoked on success)
> > >
> > > Apologies for the misunderstanding. Just to be clear, you prefer
> > > indexing in UpdateIndexCallbacks using the thread Tasks that also indexes
> > > in indexStdlib? In this implementation, I am calling the index action in
> > > preamble thread. I will revise it.
> > > Apologies for the misunderstanding. Just to be clear, you prefer indexing
> > > in UpdateIndexCallbacks using the thread Tasks that also indexes in
> > > indexStdlib? In this implementation, I am calling the index action in
> > > preamble thread. I will revise it.
> >
> > Yes, i guess that's the reason why I was confused while looking at the
> > code. Sorry if I give the impression that suggests doing the indexing on
> > `PreambleThread`, but I think both in my initial comment:
> >
> > >> As for AsyncTaskRunner to use, since this indexing task only depends on
> > >> the file-index, which is owned by ClangdServer, I don't think there's
> > >> any need to introduce a new taskrunner into TUScheduler and block its
> > >> destruction. We can just re-use the existing TaskRunner inside
> > >> parsingcallbacks, in which we run stdlib indexing tasks.
> >
> > and in the follow up;
> >
> > >> I think we can just change the signature for PreambleParsedCallback to
> > >> pass along refcounted objects. forgot to mention in the first comment,
> > >> but we should also change the CanonicalIncludes to be a shared_ptr so
> > >> that it can outlive the PreambleData. We should invoke the callback
> > >> inside buildPreamble after a successful build. Afterwards we should also
> > >> change the signature for onPreambleAST to take AST, PP and
> > >> CanonicalIncludes as ref-counted objects again and PreambleThread::build
> > >> should just forward objects received from PreambleParsedCallback.
> > >> Afterwards inside the UpdateIndexCallbacks::onPreambleAST we can just
> > >> invoke indexing on Tasks if it's present or synchronously in the absence
> > >> of it.
> >
> > I was pointing towards running this inside the `Tasks` in
> > `UpdateIndexCallbacks`.
> >
> > ---
> >
> > There's definitely some upsides to running that indexing on the preamble
> > thread as well (which is what we do today) but I think the extra sequencing
> > requirements (make sure to first notify the ASTPeer and then issue preamble
> > callbacks) we put into TUScheduler (which is already quite complex) is
> > probably not worth it.
> > > Apologies for the misunderstanding. Just to be clear, you prefer indexing
> > > in UpdateIndexCallbacks using the thread Tasks that also indexes in
> > > indexStdlib? In this implementation, I am calling the index action in
> > > preamble thread. I will revise it.
> >
> > Yes, i guess that's the reason why I was confused while looking at the
> > code. Sorry if I give the impression that suggests doing the indexing on
> > `PreambleThread`, but I think both in my initial comment:
> >
> > >> As for AsyncTaskRunner to use, since this indexing task only depends on
> > >> the file-index, which is owned by ClangdServer, I don't think there's
> > >> any need to introduce a new taskrunner into TUScheduler and block its
> > >> destruction. We can just re-use the existing TaskRunner inside
> > >> parsingcallbacks, in which we run stdlib indexing tasks.
> >
> > and in the follow up;
> >
> > >> I think we can just change the signature for PreambleParsedCallback to
> > >> pass along refcounted objects. forgot to mention in the first comment,
> > >> but we should also change the CanonicalIncludes to be a shared_ptr so
> > >> that it can outlive the PreambleData. We should invoke the callback
> > >> inside buildPreamble after a successful build. Afterwards we should also
> > >> change the signature for onPreambleAST to take AST, PP and
> > >> CanonicalIncludes as ref-counted objects again and PreambleThread::build
> > >> should just forward objects received from PreambleParsedCallback.
> > >> Afterwards inside the UpdateIndexCallbacks::onPreambleAST we can just
> > >> invoke indexing on Tasks if it's present or synchronously in the absence
> > >> of it.
> >
> > I was pointing towards running this inside the `Tasks` in
> > `UpdateIndexCallbacks`.
> >
> > ---
> >
> > There's definitely some upsides to running that indexing on the preamble
> > thread as well (which is what we do today) but I think the extra sequencing
> > requirements (make sure to first notify the ASTPeer and then issue preamble
> > callbacks) we put into TUScheduler (which is already quite complex) is
> > probably not worth it.
>
> Thanks again for the review. Updated the diff such that indexing is now in
> IndexTasks. AFIK, there will be a single thread that will now manage indexing
> of preamble for all the opened TUs. Please let me know if you see any issues
> there.
> There's definitely some upsides to running that indexing on the preamble
> thread as well (which is what we do today) but I think the extra sequencing
> requirements (make sure to first notify the ASTPeer and then issue preamble
> callbacks) we put into TUScheduler (which is already quite complex) is
> probably not worth it.
@kadircet I'm sorry for interfering with the review but could you please
elaborate what are the downsides of current approach when preamble indexing
happening off critical path but on the same thread? The benefits of such
approach that preamble marked available earlier and compilation of the main
file starts earlier in parallel with indexing preamble. Also such approach
eliminates potential race condition between preamble thread and indexing
thread. Code in
[PrecompiledPreamble::Build](https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/PrecompiledPreamble.cpp#L551-L577)
accesses AST for preamble and if indexing is running in separate thread it
will be race condition. If we schedule indexing after
PrecompiledPreamble::Build e still will need to be very careful to don't access
anything in preamble AST from PreambleThread after the task is scheduled. The
only downside of running indexing on PreambleThread is that we cannot start
building new preamble while previous one is indexing but I think it might be
positive thing to throttle preamble building.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148088/new/
https://reviews.llvm.org/D148088
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
