Skip to content
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

Jump to bottom

x/tools: unify I/O semaphores #25017

Open
josharian opened this issue Apr 23, 2018 · 8 comments
Open

x/tools: unify I/O semaphores #25017

josharian opened this issue Apr 23, 2018 · 8 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@josharian
Copy link
Contributor

grepping for the word "semaphore" reveals five separate I/O semaphores in x/tools. Some of them could easily be used in the same executable, e.g. those in go/loader, go/buildutil, refactor/importgraph, and cmd/guru. This makes them less effective.

I suggest we add a single exported semaphore to x/tools, which all x/tools packages can import and use, thus providing an actual process-wide limit on I/O concurrency. It could be in an internal package or not; I don't feel strongly.

cc @bradfitz @alandonovan @kevinburke for opinions
@josharian josharian added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 23, 2018
@josharian josharian added this to the Unreleased milestone Apr 23, 2018
@josharian
Copy link
Contributor Author

And searching for make(chan struct{}, finds three more.

@kevinburke
Copy link
Contributor

I don't feel strongly about it! I actually maintain a semaphore package but don't care if we use it. https://github.com/kevinburke/semaphore

@alandonovan
Copy link
Contributor

An internal package sounds fine. If I could redo x/tools, I would export a fraction of the APIs it currently has.

@josharian
Copy link
Contributor Author

Great, will do. Anyone want to bikeshed API details before I send a CL? For lack of better names, perhaps:

Package: golang.org/x/tools/internal/iosem

Exported API: Acquire() and Release()

Allowed concurrency would be fixed at 20.

Usage would look like:

iosem.Acquire()
// do work
iosem.Release()

@josharian
Copy link
Contributor Author

Alternative package names: iolimit, iogate. Alternative func names: Enter/Leave, Enter/Exit.

@gopherbot
Copy link

Change https://golang.org/cl/108878 mentions this issue: cmd/guru: parallelize loop in globalReferrersPkgLevel

@agnivade
Copy link
Contributor

iosem and .Acquire()/.Release() sounds fine to me.

Should we perhaps expose the concurrency ? Or make it fixed at 20 ?

@josharian
Copy link
Contributor Author

We can expose the concurrency later if there is a reason to. We use 20 most other places in x/tools, and hard-coding it for now is simpler.

gopherbot pushed a commit to golang/tools that referenced this issue May 1, 2018
@josharian
cmd/guru: parallelize loop in globalReferrersPkgLevel
d4c6246 
This change parallelizes the outer loop in globalReferrersPkgLevel,
which loops over packages to inspect.

There is also an easily parallelizable inner loop.
However, parallelizing it adds complication
(deffiles needs a mutex, inQueryPackage requires a wait group)
and offers only a 2% speed-up.

Benchmarks for this change, looking for encoding/json.MarshalIndent:

name       old time/op       new time/op       delta
Referrers        5.31s ± 2%        4.67s ± 3%  -11.95%  (p=0.000 n=10+10)

name       old user-time/op  new user-time/op  delta
Referrers        15.9s ± 2%        16.5s ± 3%   +3.71%  (p=0.000 n=10+10)

name       old sys-time/op   new sys-time/op   delta
Referrers        15.7s ± 3%        16.1s ± 3%   +2.73%  (p=0.011 n=10+10)


Fixes golang/go#24272
Updates golang/go#25017


This work supported by Sourcegraph.

Change-Id: I5dcda9017103cdff59d0ffdf5e87d2c2c955a33a
Reviewed-on: https://go-review.googlesource.com/108878
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@josharian @kevinburke @agnivade @alandonovan @gopherbot