Adds RmetaLinkCache a per-link cache that uses path as the key of dec…#158194
Open
mehdiakiki wants to merge 1 commit into
Open
Adds RmetaLinkCache a per-link cache that uses path as the key of dec…#158194mehdiakiki wants to merge 1 commit into
mehdiakiki wants to merge 1 commit into
Conversation
…oded lib.rmeta-link archive members, and routes add_archive read through it so each rlib link metadata is decoded at most once per link. This is a demand that originated from the discussion in rust-lang#156735 and we split it out as its own PR. It gives that PR a decode once path tp read instead of reparsing each rlib per crate once native_lib_filenames moves to a link time read.
Collaborator
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Collaborator
|
Contributor
Author
|
|
||
| impl RmetaLinkCache { | ||
| pub fn get_or_insert_with( | ||
| &self, |
Contributor
There was a problem hiding this comment.
Suggested change
| &self, | |
| &mut self, |
Any specific reason to use &self and RefCell instead of &mut and no RefCell?
| &self, | ||
| rlib_path: &Path, | ||
| load: impl FnOnce() -> Option<RmetaLink>, | ||
| ) -> Option<Rc<RmetaLink>> { |
Contributor
There was a problem hiding this comment.
Suggested change
| ) -> Option<Rc<RmetaLink>> { | |
| ) -> Option<&RmetaLink> { |
Is Rc necessary?
The current use in add_archive doesn't seem to require it.
| return cached.clone(); | ||
| } | ||
| let loaded = load().map(Rc::new); | ||
| self.cache.borrow_mut().insert(rlib_path.to_path_buf(), loaded.clone()); |
Contributor
There was a problem hiding this comment.
Suggested change
| self.cache.borrow_mut().insert(rlib_path.to_path_buf(), loaded.clone()); | |
| self.cache.borrow_mut().entry(rlib_path).or_insert_with(...); |
Can simplify this and avoid the double lookup by using the entry API.
| let _timer = sess.timer("link_binary"); | ||
| let output_metadata = sess.opts.output_types.contains_key(&OutputType::Metadata); | ||
| let mut tempfiles_for_stdout_output: Vec<PathBuf> = Vec::new(); | ||
| let rmeta_link_cache = rmeta_link::RmetaLinkCache::default(); |
Contributor
There was a problem hiding this comment.
Suggested change
| let rmeta_link_cache = rmeta_link::RmetaLinkCache::default(); | |
| let rmeta_link_cache = RmetaLinkCache::default(); |
Nit: imports (in archive.rs too).
|
|
||
| pub enum AddArchiveKind<'a> { | ||
| Rlib(/*skip*/ &'a dyn Fn(&str, ArchiveEntryKind) -> bool), | ||
| Rlib { skip: &'a dyn Fn(&str, ArchiveEntryKind) -> bool, cache: &'a rmeta_link::RmetaLinkCache }, |
Contributor
There was a problem hiding this comment.
Suggested change
| Rlib { skip: &'a dyn Fn(&str, ArchiveEntryKind) -> bool, cache: &'a rmeta_link::RmetaLinkCache }, | |
| Rlib(&'a RmetaLinkCache, /*skip*/ &'a dyn Fn(&str, ArchiveEntryKind) -> bool), |
Style nit: not enough fields to convert to named fields, and closures usually go last.
Contributor
|
I thought about using |
Collaborator
|
Reminder, once the PR becomes ready for a review, use |
Contributor
Author
|
Ok will address the comments above! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
RmetaLinkCachea per link and path keyed cache of decodedlib.rmeta-linkarchive members and routes the add_archive read through it so that each rlib link metadata is decoded only one per link at most.This is a request that originated from the discussion in #156735 and we split it out as its own PR. It gives that PR a decode once path to read instead of reparsing each
rlibper crate, and this will be in effect oncenative_lib_filenamesmoves to a link-time read.Part of #138243