Skip to content

Navigation Menu

Sign in
Sign up

Add concurrency to mdformatter - #63

Open
saswatamcode wants to merge 1 commit into
bwplotka:main from
saswatamcode:concurrency
Open

Add concurrency to mdformatter #63
saswatamcode wants to merge 1 commit into
bwplotka:main from
saswatamcode:concurrency

Conversation

@saswatamcode

@saswatamcode saswatamcode commented Jul 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR parallelizes the file processing loop in mdformatter for faster formatting and link checking using WaitGroup by spawning a goroutine for each file. (This also changes CLI spinner behavior as filenames cannot be shown due to multiple files being processed at once)

Some caveats,

  • Panic during cancellation sometimes due to v.c.Wait()(flaky, maybe due to internal colly WaitGroup)
  • Need some way to limit number of file processing goroutines
 

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

@bwplotka bwplotka left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Thanks, some comments.

f := New(ctx, opts...)
b := bytes.Buffer{}
// TODO(bwplotka): Add concurrency (collector will need to redone).
errorChannel := make(chan error)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errorChannel := make(chan error)
errorCh := make(chan error)

saswatamcode reacted with thumbs up emoji
spin.Message(fn + "...")
}
errs.Add(func() error {
go func(fn string) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use https://pkg.go.dev/golang.org/x/sync/errgroup maybe, so semantics are easier.

saswatamcode reacted with thumbs up emoji
return
}
v.destFutures[k] = &futureResult{cases: 1, resultFn: func() error { return nil }}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a race during replacement operation.

saswatamcode reacted with thumbs up emoji
func (v RoundTripValidator) IsValid(k futureKey, r *validator) (bool, error) {
// Result will be in future.
r.destFutures[k].resultFn = func() error { return r.remoteLinks[k.dest] }
prevResult, _ := r.destFutures.LoadAndDelete(k)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot's of races when we delete, unfortunately.. we need to make this operation locked potentially with Mutex.

But it has to be the full operation Locked:

  1. Checking if there is entry in map
  2. If no, create new future, and schedule colly
  3. If yes, grab reference to future

We might need to check IsValid interface, if this actually allows us to do this 🤔

saswatamcode reacted with thumbs up emoji

Copy link
Copy Markdown
Owner

Still comments not addressed, I believe?
saswatamcode reacted with thumbs up emoji

bwplotka commented Jun 7, 2023

Copy link
Copy Markdown
Owner

Any progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwplotka bwplotka Awaiting requested review from bwplotka

At least 1 approving review is required to merge this pull request.

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /