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

x/tools: unify I/O semaphores #25017

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

Comments

Projects
None yet
5 participants
@josharian
Copy link
Contributor

josharian commented Apr 23, 2018

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 this to the Unreleased milestone Apr 23, 2018

@josharian

This comment has been minimized.

Copy link
Contributor Author

josharian commented Apr 23, 2018

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

@kevinburke

This comment has been minimized.

Copy link
Contributor

kevinburke commented Apr 23, 2018

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

This comment has been minimized.

Copy link
Contributor

alandonovan commented Apr 23, 2018

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

@josharian

This comment has been minimized.

Copy link
Contributor Author

josharian commented Apr 23, 2018

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

This comment has been minimized.

Copy link
Contributor Author

josharian commented Apr 23, 2018

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Apr 23, 2018

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

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Apr 23, 2018

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

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

@josharian

This comment has been minimized.

Copy link
Contributor Author

josharian commented Apr 23, 2018

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

cmd/guru: parallelize loop in globalReferrersPkgLevel
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.