Migrate indexHieFile progress notification to ProgressReporting API #4205

soulomoon · 2024-05-02T23:27:21Z

What's done

Refactor ProgressReporting to allow external state management
Migrate indexHieFile progress to ProgressReporting API
Add Note [ProgressReporting API and InProgressState] to demonstrate the current status

michaelpj · 2024-05-03T11:18:13Z

Could we refactor this to actually use the helpers from lsp for running progress sessions? I think that should handle all of this properly, rather than us re-implementing it here?

wz1000 · 2024-05-03T11:20:48Z

This code manages its progress token in a somewhat unique way which is not easy to statically scope with the lsp progress functions.

wz1000 · 2024-05-03T11:22:14Z

ghcide/src/Development/IDE/Core/Compile.hs

-              _ <- LSP.sendRequest LSP.SMethod_WindowWorkDoneProgressCreate (LSP.WorkDoneProgressCreateParams u) (const $ pure ())
+              b <- liftIO newBarrier
+              void $ LSP.sendRequest LSP.SMethod_WindowWorkDoneProgressCreate (LSP.WorkDoneProgressCreateParams u) $ liftIO . signalBarrier b
+              ready <- liftIO $ waitBarrier b


I don't know if its a good idea to delay indexing by an unbounded amount of time waiting for the client to respond with a progress token. Perhaps the progress reporting should be on another thread which doesn't block indexing.

yes, but indexHieFile would return without reportProgress actually report done🤔. I am not sure how does it do to our test, I'll see how it goes.

soulomoon · 2024-05-03T15:30:10Z

Instead of a long runner task, it is more like tunes of small tasks dynamically created only finished when the all the pendding tasks are done. Hard to fit this in helpers from lsp.
But we still can if we are handling this in a seperate thread, might need serveral pipes though. And hard to say if it would make thing simpler🤔

soulomoon · 2024-05-04T23:46:44Z

I've spawn a new thread to run the indexing progress update, send through TQueue. 🤔 Is it a good idea to do so？

michaelpj · 2024-05-19T15:06:56Z

I think we can probably do something like #4218 now

soulomoon · 2024-06-12T07:53:04Z

I think we can probably do something like #4218 now

It seems, here we are using a push update model.
And in #4218, it is a pull update model for every interval.
Maybe we can unify to use a pull update model?

Also, I am wondering if we can make Development.IDE.Core.ProgressReporting more general, so we can actually reuse it.

michaelpj · 2024-06-12T08:55:34Z

Also, I am wondering if we can make Development.IDE.Core.ProgressReporting more general, so we can actually reuse it.

Yes, I think there's almost something there. It's something like a standalone counter that also has some triggers for killing and restarting it?

ghcide/src/Development/IDE/Core/Compile.hs

michaelpj

Looks plausible, I wonder if we can simplify ProgressReporting and just have one way to do it.

ghcide/src/Development/IDE/Core/ProgressReporting.hs

michaelpj · 2024-06-17T09:14:41Z

ghcide/src/Development/IDE/Core/ProgressReporting.hs


 data ProgressEvent
-    = KickStarted
-    | KickCompleted
+  = ProgressStarted


michaelpj · 2024-06-17T09:20:47Z

ghcide/src/Development/IDE/Core/Compile.hs

+              Nothing -> do
+                  tt <- progressReportingOutsideState
+                    (liftM2 (+) (HashMap.size <$> readTVar indexPending) (readTVar indexCompleted))
+                    (readTVar indexCompleted)


This seems a bit backwards to me? Can we not use the usual ProgressReporting and then call progressUpdate etc. instead of touching these TVars in the indexing code?

Okay, I think I get it. Now I wonder if it would be simpler to make the other progress tracker work this way?

As along as they are already tracking the progress status external at some T*** varaibles.

But I take a look at the use case at kick, the state(todo count,done count) tracking is at defineEarlyCutoff' and independent for each file. No easy way to switch the Kick progress tracker work this way.

Right, but when we create the progress tracker we could set that stuff up externally, so that the ProgressReporting doesn't see it. Let me see if I can just write it down.

Here: 2ad92a0

If we did this then I think indexHieFile could just use progressCounter directly?

Great, I've extracted it to standalone now
@michaelpj

I'll see if it fit in.

then I think if you export it you can just use it in indexHieFile

If using it barely in indexHieFile, we still need some kind of wrapping around progressCounter. async and cancel the progressCounter.

Yeah, we do need that. But we could just do it locally. Or maybe you want to have a standalone concept of a "resumable/cancellable progress counter" and then just put the file-tracking on top for the compilation progress tracker?

May be the latter, I am moving up the indexHieFile reporting initialization to shakeOpen as that of the Kick reporting. Should be much cleaner than the preivous version.

"resumable/cancellable progress counter" need an additional start Event that won't replace the existing progress reporting.

michaelpj · 2024-06-17T09:26:03Z

ghcide/src/Development/IDE/Core/ProgressReporting.hs

-    = KickStarted
-    | KickCompleted
+  = ProgressStarted
+  | ProgressCompleted


Can we just get rid of progressStop, given that it just does the same thing as progressUpdate ProgressCompleted?

Kick progress tracking working differently than indexHIeFile.

HLS begins: Progress tracking started

loop:

Kick start ProgressStarted

Kick end ProgressCompleted

HLS shutdown: Progress tracking Stop

Oh yes, I missed that the state update is different, ProgressCompleted goes back to NotStarted. It's not clear that that gets us much, though 🤔

NotStarted and Stopped are the same state, except you can't start again from Stopped. But why does that even matter? In both cases you stop the session, it's not like Stopped cleans up any more resources!

So the way it is structured we get a restartable progress tracker, which is a reasonable thing to have when you keep a single reference to it alive for a long time.. But I think it could be simpler.

So the way it is structured we get a restartable progress tracker

Yes, I think it is intended for multiple Kick's.

or we need to initialize progress tracker at each kick.

So basically:
restartable wrapped progress tracker versus progress tracker used only once.

I guess, Stopped state is intended to forbit us to start the progress tracker again after the we shutdown hls

That doesn't seem actually useful, though. I'd be inclined to remove it.

ghcide/src/Development/IDE/Core/Compile.hs

ghcide/src/Development/IDE/Core/ProgressReporting.hs

Co-authored-by: Michael Peyton Jones <me@michaelpj.com>

soulomoon · 2024-06-17T12:28:00Z

Now it becomes resumable/restartable/cancellable progress reporting

We might want to renamed that

ProgressStarted -> ProgressNewStarted: cancel the old one
ProgressTryToStart -> ProgressStarted: won't cancel the old one

soulomoon · 2024-06-18T11:43:47Z

After introducing new event capturing more progress reporting logic, we are able to change the indexProgressToken :: Var (Maybe LSP.ProgressToken) to indexProgressReporting :: ProgressReporting IO. Ready to be reviewed again @michaelpj

michaelpj

Looks good, maybe just write down some of what we've learned!

michaelpj · 2024-06-19T16:55:03Z

ghcide/src/Development/IDE/Core/ProgressReporting.hs

-    = KickStarted
-    | KickCompleted
+  = ProgressStarted
+  | ProgressCompleted


That doesn't seem actually useful, though. I'd be inclined to remove it.

ghcide/src/Development/IDE/Core/ProgressReporting.hs

michaelpj · 2024-06-19T16:58:02Z

ghcide/src/Development/IDE/Core/ProgressReporting.hs

+        doneVar    :: TVar Int,
+        currentVar :: STM.Map NormalizedFilePath Int
+      }
+  | InProgressStateOutSide


I still find this split a bit weird, but we've spent enough time on this!

yes, especially we have inProgress that is useless InProgressStateOutSide.
Maybe in some following ups, we can come up with alternative design such as using GADTs to eliminate this discrepancy

michaelpj · 2024-06-19T16:59:22Z

ghcide/src/Development/IDE/Core/Compile.hs

-                  , _percentage = Nothing
-                  }
-
+    pre = progressUpdate indexProgressReporting ProgressStarted


I had actually thought we could just initialize a progressCounter here and use it just in the scope of this function, but this works too!

ghcide/src/Development/IDE/Core/ProgressReporting.hs

michaelpj · 2024-06-20T08:34:36Z

Needs conflicts fixed then good to go

* master: (36 commits) Migrate indexHieFile progress notification to ProgressReporting API (haskell#4205) Remove final allow-newer for 9.10 (haskell#4329) Remove unused exactprint dep More stylish Use newer cabal-fmt, partially lift ghc version restriction stylish Cleanup CI configs and cabal files More no-op code cleanup Remove no-longer-needed compat code, remove unused stuff Remove pre-multi component junk for GHC <= 9.2 Fix stylish Fix a few things Remove from CI Update docs Fix loss of 9.2 GHC version More CPP WIP evaluate CPP Prepare release 2.9.0.0 (haskell#4319) Add completion for import fields in cabal files (haskell#4305) Refine GHC deprecation policy (haskell#3438) ...

soulomoon force-pushed the soulomoon/wait-for-token-indexHieFile branch from 1440f35 to d70acdb Compare May 2, 2024 23:36

soulomoon changed the title ~~Wait For token using Barrier when indexHieFile~~ Wait for progress token using Barrier when indexHieFile May 3, 2024

soulomoon marked this pull request as ready for review May 3, 2024 01:21

soulomoon requested a review from pepeiborra as a code owner May 3, 2024 01:21

soulomoon requested review from wz1000, michaelpj and fendor May 3, 2024 01:21

wz1000 reviewed May 3, 2024

View reviewed changes

soulomoon marked this pull request as draft May 4, 2024 13:43

soulomoon marked this pull request as ready for review May 4, 2024 23:43

soulomoon added the status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet label May 5, 2024

soulomoon force-pushed the soulomoon/wait-for-token-indexHieFile branch from 56f43e6 to 54fdce3 Compare June 13, 2024 08:34

soulomoon requested review from joyfulmantis, peterbecich and ozkutuk as code owners June 13, 2024 08:34

swtich to general progress

8981601

soulomoon force-pushed the soulomoon/wait-for-token-indexHieFile branch from 54fdce3 to 8981601 Compare June 13, 2024 08:38

soulomoon commented Jun 13, 2024

View reviewed changes

ghcide/src/Development/IDE/Core/Compile.hs Outdated Show resolved Hide resolved

soulomoon commented Jun 13, 2024

View reviewed changes

ghcide/src/Development/IDE/Core/Compile.hs Outdated Show resolved Hide resolved

soulomoon added 4 commits June 13, 2024 16:46

fix

ae83626

format

8800d2a

add progressReportingOutsideState

f3cc5b0

format with stylish

dde4c1d

soulomoon commented Jun 17, 2024

View reviewed changes

ghcide/src/Development/IDE/Core/Compile.hs Show resolved Hide resolved

michaelpj reviewed Jun 17, 2024

View reviewed changes

soulomoon and others added 6 commits June 17, 2024 06:17

add comment

43abadd

adjust to use progressCounter

0c9325e

Extract progressCounter

e83f3bc

Co-authored-by: Michael Peyton Jones <me@michaelpj.com>

IO switch to m in progressUpdate

ad25018

format

1a75a36

try to start at the beginning

fb44ba8

soulomoon added 3 commits June 17, 2024 08:43

rename

b3a4456

stylish

4671143

add shutdown

477e481

soulomoon requested a review from michaelpj June 18, 2024 13:57

michaelpj approved these changes Jun 19, 2024

View reviewed changes

soulomoon added 3 commits June 20, 2024 13:58

Merge branch 'master' into soulomoon/wait-for-token-indexHieFile

072b882

add note

51acba7

add Note

0b3808f

Merge branch 'master' into soulomoon/wait-for-token-indexHieFile

1405cb7

soulomoon changed the title ~~Wait for progress token using Barrier when indexHieFile~~ Migrate indexHieFile progress notification to ProgressReporting API Jun 20, 2024

soulomoon enabled auto-merge (squash) June 20, 2024 17:00

fix

d21ba21

soulomoon added merge me Label to trigger pull request merge and removed merge me Label to trigger pull request merge status: needs review This PR is ready for review labels Jun 20, 2024

Merge branch 'master' into soulomoon/wait-for-token-indexHieFile

4f3828c

soulomoon merged commit f523690 into haskell:master Jun 20, 2024
36 checks passed

soulomoon mentioned this pull request Jun 25, 2024

Formalize the ProgressReporting Type #4335

Merged

Migrate indexHieFile progress notification to ProgressReporting API #4205

Migrate indexHieFile progress notification to ProgressReporting API #4205

Conversation

soulomoon commented May 2, 2024 • edited Loading

michaelpj commented May 3, 2024

wz1000 commented May 3, 2024

Choose a reason for hiding this comment

soulomoon May 3, 2024 • edited Loading

Choose a reason for hiding this comment

soulomoon commented May 3, 2024 • edited Loading

soulomoon commented May 4, 2024 • edited Loading

michaelpj commented May 19, 2024

soulomoon commented Jun 12, 2024 • edited Loading

michaelpj commented Jun 12, 2024

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

soulomoon Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon commented Jun 17, 2024 • edited Loading

soulomoon commented Jun 18, 2024 • edited Loading

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Jun 20, 2024

soulomoon commented May 2, 2024 •

edited

Loading

soulomoon May 3, 2024 •

edited

Loading

soulomoon commented May 3, 2024 •

edited

Loading

soulomoon commented May 4, 2024 •

edited

Loading

soulomoon commented Jun 12, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon Jun 17, 2024 •

edited

Loading

soulomoon commented Jun 17, 2024 •

edited

Loading

soulomoon commented Jun 18, 2024 •

edited

Loading

soulomoon Jun 20, 2024 •

edited

Loading