-
Notifications
You must be signed in to change notification settings - Fork 86
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
refactor(*): add source.as_bytes() #454
Conversation
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.resolve_in_path_or_cwd() | ||
.next() | ||
.context("module not found")?; | ||
Ok(Cow::Owned(std::fs::read(path)?)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: We optimize for no copy in the Oci
case by using Cow
, but we read the file every time in the File
case. Might be premature optimiation, but the file content could be cached in memory here to avoid multiple reads.
I'm fine with leaving it as it is because the unoptimized File
case would be the legacy workflow. Also we could revisit if we ever observe any perforance bottleneck here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR is a preferred way to refactor the usage of
Source
into aas_bytes()
function in Context. It does not change the APIs of the Engine trait. Closes #448