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

fix: re-use embedded union reader if possible #2814

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

willmurphyscode
Copy link
Contributor

Supersedes #2811, at least for now.

Previously, because file.LocationReadCloser embeds a ReadCloser that might be a UnionReader, but doesn't implement the interface itself, the type assertion would fall and Syft would fall back to io.ReadAll to enable seeking on the underlying reader, resulting in a potentially large extra allocation.

Instead, check whether the passed ReadCloser is a
file.LocationReadCloser, and if so, try to use the embedded ReadCloser as a UnionReader.

Previously, because file.LocationReadCloser embeds a ReadCloser that
might be a UnionReader, but doesn't implement the interface itself, the
type assertion would fall and Syft would fall back to io.ReadAll to
enable seeking on the underlying reader, resulting in a potentially
large extra allocation.

Instead, check whether the passed ReadCloser is a
file.LocationReadCloser, and if so, try to use the embedded ReadCloser
as a UnionReader.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode
Copy link
Contributor Author

Here's profiling of this branch scanning a giant image (graph comparable to those attached to superseded PR #2811

image

This fix results in only a few more lines and results in many more memory savings that the other PR.

It's worth noting that the other PR would help in the case that Syft is used as a library, and is being invoked on a source of io.Readers that isn't stereoscope.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple notes about the test, but overall this looks 👍

syft/internal/unionreader/union_reader_test.go Outdated Show resolved Hide resolved
syft/internal/unionreader/union_reader_test.go Outdated Show resolved Hide resolved
Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode willmurphyscode merged commit d3310a1 into main Apr 26, 2024
11 checks passed
@willmurphyscode willmurphyscode deleted the fix-check-for-location-read-closer branch April 26, 2024 14:21
@willmurphyscode willmurphyscode self-assigned this Apr 26, 2024
@willmurphyscode willmurphyscode added the bug Something isn't working label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants