-
Notifications
You must be signed in to change notification settings - Fork 81
Load packages concurrently in filesystem indexers #1489
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
Changes from 1 commit
6ed5f33
c9eff98
01407b7
024d5d3
9e0a0d0
9241261
4e78562
6612363
21ec258
44f3419
6dcc859
5aea1c6
a62499d
e9ca9a6
fcb290e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,11 +7,14 @@ package packages | |
| import ( | ||
| "archive/zip" | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "slices" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/Masterminds/semver/v3" | ||
|
|
@@ -235,40 +238,103 @@ func (i *FileSystemIndexer) getPackagesFromFileSystem(ctx context.Context) (Pack | |
| name string | ||
| version string | ||
| } | ||
| type packageJob struct { | ||
| position int | ||
| path string | ||
| } | ||
| numWorkers := runtime.GOMAXPROCS(0) / 2 | ||
| mu := sync.Mutex{} | ||
| packagesFound := make(map[packageKey]struct{}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why using here an empty struct? could we use a bool? and chech its found (by key) and true?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, I just interested in whether or not the key is present. I think it is not needed to use boolean. An advantage of using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i need a 💡 for reacting , thanks! learned something today |
||
| jobChan := make(chan packageJob) | ||
| errChan := make(chan error) | ||
|
|
||
| var pList Packages | ||
| count := 0 | ||
| for _, basePath := range i.paths { | ||
| packagePaths, err := i.getPackagePaths(basePath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| count += len(packagePaths) | ||
| } | ||
| pList := make(Packages, count) | ||
|
|
||
| i.logger.Info("Searching packages in " + basePath) | ||
| for _, path := range packagePaths { | ||
| p, err := NewPackage(i.logger, path, i.fsBuilder) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("loading package failed (path: %s): %w", path, err) | ||
| } | ||
| var wg sync.WaitGroup | ||
|
|
||
| key := packageKey{name: p.Name, version: p.Version} | ||
| if _, found := packagesFound[key]; found { | ||
| i.logger.Debug("duplicated package", | ||
| zap.String("package.name", p.Name), | ||
| zap.String("package.version", p.Version), | ||
| zap.String("package.path", p.BasePath)) | ||
| continue | ||
| i.logger.Info("Searching packages in filesystem", zap.String("indexer", i.label)) | ||
| // Start workers | ||
| for range numWorkers { | ||
| wg.Add(1) | ||
| go func(logger *zap.Logger, fsBuilder FileSystemBuilder) { | ||
| defer wg.Done() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, consider moving the workers pool logic to different functions for clarity. Something like this is done in https://github.com/elastic/package-registry/pull/1335/files#diff-8ea6771aea0aa89d25c6b29227dbc22db28f067930f7b9fc9921134c66c744b5R50.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll check it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the code to use the same functions as your link. |
||
| for pkgJob := range jobChan { | ||
| p, err := NewPackage(logger, pkgJob.path, fsBuilder) | ||
| if err != nil { | ||
| errChan <- fmt.Errorf("loading package failed (path: %s): %w", pkgJob.path, err) | ||
| continue | ||
| } | ||
|
|
||
| key := packageKey{name: p.Name, version: p.Version} | ||
| func() { | ||
|
jsoriano marked this conversation as resolved.
Outdated
|
||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| if _, found := packagesFound[key]; found { | ||
| i.logger.Debug("duplicated package", | ||
| zap.String("package.name", p.Name), | ||
| zap.String("package.version", p.Version), | ||
| zap.String("package.path", p.BasePath)) | ||
| return | ||
| } | ||
|
|
||
| packagesFound[key] = struct{}{} | ||
| pList[pkgJob.position] = p | ||
|
|
||
| i.logger.Debug("found package", | ||
| zap.String("package.name", p.Name), | ||
| zap.String("package.version", p.Version), | ||
| zap.String("package.path", p.BasePath)) | ||
|
|
||
| }() | ||
| } | ||
| }(i.logger, i.fsBuilder) | ||
| } | ||
|
|
||
| packagesFound[key] = struct{}{} | ||
| pList = append(pList, p) | ||
| count = 0 | ||
| for _, basePath := range i.paths { | ||
| packagePaths, err := i.getPackagePaths(basePath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, path := range packagePaths { | ||
| jobChan <- packageJob{position: count, path: path} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need the paths to keep the original positions? 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought to keep the same ordering that it was now, to ensure there are no changes in the list generated. |
||
| count++ | ||
| } | ||
| } | ||
| close(jobChan) | ||
|
|
||
| wg.Wait() | ||
|
|
||
| i.logger.Debug("found package", | ||
| zap.String("package.name", p.Name), | ||
| zap.String("package.version", p.Version), | ||
| zap.String("package.path", p.BasePath)) | ||
| close(errChan) | ||
|
|
||
| var multiErr error | ||
| for err := range errChan { | ||
| multiErr = errors.Join(multiErr, err) | ||
| } | ||
|
|
||
| if multiErr != nil { | ||
| return nil, multiErr | ||
| } | ||
|
|
||
| // Remove null entries in case of duplicated packages | ||
| current := 0 | ||
| for _, p := range pList { | ||
| if p != nil { | ||
| pList[current] = p | ||
| current++ | ||
| } | ||
| } | ||
| pList = pList[:current] | ||
| i.logger.Info("Searching packages in filesystem done", zap.String("indexer", i.label)) | ||
|
|
||
| return pList, nil | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try directly to use GOMAXPROCS instead of half of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only used half of them because I was being conservative. But, it should not be any problem on setting GOMAXPROCS instead.