-
Notifications
You must be signed in to change notification settings - Fork 11
wittier caching and exe getting for golang #150
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
base: master
Are you sure you want to change the base?
Conversation
@marcin-ol can you please review it? I'll review afterwards |
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.
Completed a review.
Please also note remarks to the issue intel/gprofiler#696 .
@functools.lru_cache(maxsize=4096) | ||
def get_version_hidden_in_exe(elf_path: str, process_start_time: float) -> Optional[str]: | ||
process = None | ||
for pid in pids(): |
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.
Consider using psutil.process_iter()
, as it's caching created Process
instances, and yields only live processes.
Note that this method reads additional process attributes - might limit them to create_time
and exe
.
process = None | ||
for pid in pids(): | ||
if Process(pid).create_time() == process_start_time: | ||
process = Process(pid) |
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.
Might want to break the iteration early at this point.
def get_version_hidden_in_exe(elf_path: str, process_start_time: float) -> Optional[str]: | ||
process = None | ||
for pid in pids(): | ||
if Process(pid).create_time() == process_start_time: |
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.
Is this condition enough? create_time
has one second granularity, I'm concerned about raising NoSuchProcess
few lines later.
except: | ||
return None | ||
elf_path = f"/proc/{get_mnt_ns_ancestor(process).pid}/root{exe}" | ||
return get_version_hidden_in_exe(elf_path, process.create_time()) |
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.
Version should be cached for a path and its modification time. Depending on process' create_time will reduce the effect of caching - every new process has new create time.
It won't be possible for a new process to reuse version info cached earlier.
related issue: intel/gprofiler#696