[tools] gopls/internal/filecache: add GetDecoded for caching decoded objects
Robert Findley (Gerrit)
Robert Findley voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 12 comments![]( "Open in Gerrit")
if s, err := filecache.Get(cacheKind, key, func(data []byte) *analyzeSummary {
var s *analyzeSummary
analyzeSummaryCodec.Decode(data, &s)
return s
}); err == nil {This would make a nice toplevel function adjacent to analyzeSummaryCodec.Decode.
if s == nil { // debugging #66732
bug.Reportf("analyzeSummaryCodec.Decode yielded nil *analyzeSummary")
} else {This bug was fixed; let's delete this control flow path.
// Copy: callers may zero out summary.Actions (see decrefPreds),
// so we must not mutate the shared cached value.Yikes that is subtle
XXX
data, err := filecache.Get(exportDataKind, ph.key, filecache.Bytes)Oooh... this makes me wonder whether we could cache the pre-decoded export data in some meaningful way, independent of token.FileSet and types.Importer, that would make it cheaper to specialize it in as needed. Another day...
if data, err := filecache.Get(typerefsKind, key, filecache.Bytes); err == nil {Sadly in this case the decoding depends on s.view.pkgIndex, so we can't usefully cache it.
return packageCodec.Encode(packages)I think you can simplify the API of this package if you split this function before this step. I constructs a JSON tree, then encodes it to binary. But if it were to return the tree, it could be cached directly, and the encode step could be moved to storePackageResults just like all the others.
if data, ok = v.([]byte); !ok {I wonder why this case is necessary. Should the memCache always hold a value of type T?
return zero, bug.Errorf("filecache: kind %q used with multiple decoded types (got %T, want %T)", kind, v, zero)We can just panic here; as you say, it's a programming error.
// get reads the value for (kind, key) from disk, bypassing the
// in-memory cache."the file system."
func BugReports() (string, []bug.Bug) {FWIW I plan to remove this mechanism, which was never really useful (unlike crash telemetry, which has been so effective that now most new reports are due to hardware faults).
data, err := filecache.Get(filewatcherKind, key, filecache.Bytes)Pass a lambda that calls codec.Decode?
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 2 comments![]( "Open in Gerrit")
// Copy: callers may zero out summary.Actions (see decrefPreds),
// so we must not mutate the shared cached value.Yikes that is subtle
XXX
Agreed. I think let's just revert this one to bytes and do the decoding in line. Too subtle.
if data, ok = v.([]byte); !ok {I wonder why this case is necessary. Should the memCache always hold a value of type T?
We don't Set the value: we still Set the bytes and only replace with the decoded value on the first Get. I thought about making the API symmetric and passing in bytes to Set, but actually don't want to have any sort of dependency on pointer identities.
WDYT?
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 3 comments![]( "Open in Gerrit")
// Copy: callers may zero out summary.Actions (see decrefPreds),
// so we must not mutate the shared cached value.Yikes that is subtle
XXX
Sorry, I meant to write that better before I mailed it out!
// and upgraded in place."from raw bytes to decoded data"?
if data, ok = v.([]byte); !ok {Robert FindleyI wonder why this case is necessary. Should the memCache always hold a value of type T?
We don't Set the value: we still Set the bytes and only replace with the decoded value on the first Get. I thought about making the API symmetric and passing in bytes to Set, but actually don't want to have any sort of dependency on pointer identities.
WDYT?
I thought about making the API symmetric and passing in bytes to Set, but actually don't want to have any sort of dependency on pointer identities.
I assume you mean passing an encoder to Set. But yes that makes sense. A comment would help (see above).
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 8 comments![]( "Open in Gerrit")
if s, err := filecache.Get(cacheKind, key, func(data []byte) *analyzeSummary {
var s *analyzeSummary
analyzeSummaryCodec.Decode(data, &s)
return s
}); err == nil {This would make a nice toplevel function adjacent to analyzeSummaryCodec.Decode.
Ack -- obsolete since I backed off of caching the summary. Maybe for a later PR where we can make it obviously immutable.
if s == nil { // debugging #66732
bug.Reportf("analyzeSummaryCodec.Decode yielded nil *analyzeSummary")
} else {This bug was fixed; let's delete this control flow path.
Ok, I've cleaned up all the artifacts of this bug (looks like it was never root caused, but I see that it was closed).
// Copy: callers may zero out summary.Actions (see decrefPreds),
// so we must not mutate the shared cached value.Alan DonovanYikes that is subtle
XXX
Sorry, I meant to write that better before I mailed it out!
Yeah, I'll follow up with a CL clarifying the immutable part of the summary. That's enough to be its own CL.
I think you can simplify the API of this package if you split this function before this step. I constructs a JSON tree, then encodes it to binary. But if it were to return the tree, it could be cached directly, and the encode step could be moved to storePackageResults just like all the others.
Done
return zero, bug.Errorf("filecache: kind %q used with multiple decoded types (got %T, want %T)", kind, v, zero)We can just panic here; as you say, it's a programming error.
Done
// get reads the value for (kind, key) from disk, bypassing the
// in-memory cache.Robert Findley"the file system."
Done
func BugReports() (string, []bug.Bug) {FWIW I plan to remove this mechanism, which was never really useful (unlike crash telemetry, which has been so effective that now most new reports are due to hardware faults).
Sounds good.
data, err := filecache.Get(filewatcherKind, key, filecache.Bytes)Pass a lambda that calls codec.Decode?
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Robert Findley
| Commit-Queue | +1 |
2 comments
"from raw bytes to decoded data"?
Done
if data, ok = v.([]byte); !ok {I wonder why this case is necessary. Should the memCache always hold a value of type T?
Alan DonovanWe don't Set the value: we still Set the bytes and only replace with the decoded value on the first Get. I thought about making the API symmetric and passing in bytes to Set, but actually don't want to have any sort of dependency on pointer identities.
WDYT?
I thought about making the API symmetric and passing in bytes to Set, but actually don't want to have any sort of dependency on pointer identities.
I assume you mean passing an encoder to Set. But yes that makes sense. A comment would help (see above).
Done
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 3 comments![]( "Open in Gerrit")
func Index(files []*parsego.File, pkg *types.Package, info *types.Info) *XrefIndex {Let's call this `func NewIndex(...) *Index`.
state, err := filecache.Get(filewatcherKind, key, func(data []byte) fileState {
var state fileState
codec.Decode(data, &state)
return state
})BTW, this could be just codec.Decode2 if you add this method in frob:
```
func (codec Codec[T]) Decode2(data []byte) (x T) {
codec.Decode(data, &x)
return
}
```
The analysis.go one (now deleted) has a similar form.
Obviously it needs a better name. Perhaps Decode, Decode2 -> DecodeTo, Decode?
if err != nil && err != filecache.ErrNotFound {FYI: EMFILES and ENFILES are common errors from this operation when file descriptors are exhausted (which can happen either due to `go list`'s DiskCache.Trim, or when using gopls' inotify-based watcher.)
I plan to abstract some kind of filecache.IsTransientError(err) function so that all the callers of Get don't have to reinvent the error-filtering wheel.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 3 comments![]( "Open in Gerrit")
func Index(files []*parsego.File, pkg *types.Package, info *types.Info) *XrefIndex {Let's call this `func NewIndex(...) *Index`.
Done. Nice.
state, err := filecache.Get(filewatcherKind, key, func(data []byte) fileState {
var state fileState
codec.Decode(data, &state)
return state
})BTW, this could be just codec.Decode2 if you add this method in frob:
```
func (codec Codec[T]) Decode2(data []byte) (x T) {
codec.Decode(data, &x)
return
}
```The analysis.go one (now deleted) has a similar form.
Obviously it needs a better name. Perhaps Decode, Decode2 -> DecodeTo, Decode?
Let's do this in a separate CL. I'll send a follow-up.
if err != nil && err != filecache.ErrNotFound {FYI: EMFILES and ENFILES are common errors from this operation when file descriptors are exhausted (which can happen either due to `go list`'s DiskCache.Trim, or when using gopls' inotify-based watcher.)
I plan to abstract some kind of filecache.IsTransientError(err) function so that all the callers of Get don't have to reinvent the error-filtering wheel.
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
2 comments
// Inv: all root nodes now have a summary (#66732).Now that the bug is fixed (we believe), can we restore this with a hard panic (and updated comment)? The invariant is still important.
// An Index is a decoded cross-reference index, ready for lookups.// An Index an index of outbound symbol references for one specific package.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 2 comments![]( "Open in Gerrit")
// Inv: all root nodes now have a summary (#66732).Now that the bug is fixed (we believe), can we restore this with a hard panic (and updated comment)? The invariant is still important.
Done
// An Index is a decoded cross-reference index, ready for lookups.// An Index an index of outbound symbol references for one specific package.
Done
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |