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

go/types: make instantiation concurrency-safe #47910

Closed
findleyr opened this issue Aug 23, 2021 · 5 comments
Closed

go/types: make instantiation concurrency-safe #47910

findleyr opened this issue Aug 23, 2021 · 5 comments
Labels
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 23, 2021

Type instantiation is currently not concurrency-safe in go/types:

  • instances are expanded lazily.
  • instantiation hashing uses a package-local instanceHashing variable

See also #47729: the only reason this hasn't triggered test failures due to data races is that we lack test coverage for concurrent use in go/types, and x/tools is not yet exercising type instantiation.

CC @griesemer

@findleyr findleyr changed the title go/types: go/types: make instantiation concurrency-safe Aug 23, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 23, 2021

Change https://golang.org/cl/344390 mentions this issue: go/types: implement deduplication of instances using the Environment

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 31, 2021

Change https://golang.org/cl/346557 mentions this issue: go/types: eliminate typeHashing global variable

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Aug 31, 2021

This may be a concern for x/tools/go/ssa. Building for each package is done in its own go routine. Each will likely be instantiating types.

Loading

gopherbot pushed a commit that referenced this issue Sep 1, 2021
This is a port of CL 345929 to go/types. It is also a step toward making
instantiation concurrency-safe.

Also fix some whitespace in instantiate.go.

Updates #47910

Change-Id: Icdeb227cb83eee15da6db90daab294c8c55db601
Reviewed-on: https://go-review.googlesource.com/c/go/+/346557
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@findleyr findleyr removed this from the Backlog milestone Sep 9, 2021
@findleyr findleyr added this to the Go1.18 milestone Sep 9, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 12, 2021

Change https://golang.org/cl/349410 mentions this issue: go/types: merge Named type loading and expansion

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 12, 2021

Change https://golang.org/cl/349411 mentions this issue: go/types: eliminate Named.instPos

Loading

gopherbot pushed a commit that referenced this issue Sep 14, 2021
Named type expansion and loading were conceptually similar: a mechanism
for lazily resolving type information in a concurrency-safe manner.
Unify them into a 'resolve' method, that delegates to a resolver func to
produce type parameters, underlying, and methods.

By leveraging the sync.Once field on Named for instance expansion, we
get closer to making instance expansion concurrency-safe, and remove the
requirement that instPos guard instantiation. This will be cleaned up
in a follow-up CL.

This also fixes #47887 by causing substituted type instances to be
expanded (in the old code, this could be fixed by setting instPos when
substituting).

For #47910
Fixes #47887

Change-Id: Ifc52a420dde76e3a46ce494fea9bd289bc8aca4c
Reviewed-on: https://go-review.googlesource.com/c/go/+/349410
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot gopherbot closed this in bf26e43 Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants