Skip to content

Unify module caches#1120

Merged
wolfpld merged 12 commits into
wolfpld:masterfrom
siliceum:refactor/unify-module-caches
Oct 24, 2025
Merged

Unify module caches#1120
wolfpld merged 12 commits into
wolfpld:masterfrom
siliceum:refactor/unify-module-caches

Conversation

@Lectem
Copy link
Copy Markdown
Collaborator

@Lectem Lectem commented Jul 31, 2025

This is part of the changes we had in #1009

This is an attempt at unifying the symbol caching mechanisms between windows and linux.
It will later be the base for also storing module debug information (pdb guid, gnu_debug_id, ...).

@Lectem Lectem force-pushed the refactor/unify-module-caches branch from 663ec72 to c54b9b8 Compare August 1, 2025 19:01
@Lectem
Copy link
Copy Markdown
Collaborator Author

Lectem commented Aug 1, 2025

Rebased due to merge conflicts.

@Lectem
Copy link
Copy Markdown
Collaborator Author

Lectem commented Sep 7, 2025

Is there anything holding this PR back?
I'm planning on working on the offline resolution again in the upcoming weeks

Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
@wolfpld
Copy link
Copy Markdown
Owner

wolfpld commented Oct 8, 2025

Is there anything holding this PR back?

I don't have appropriate tools to review the diff (it's practically everything in one commit). Currently looking for some way to print the diff on the paper so I can write some notes.

Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
… be used in server, and thus not pointing to current process memory
The name was a bit misleading as it could be mistaken to mean "The cache contains the address" and not as "has an image with this start address". ie: that it could be mistaken to do GetImageForAddress( startAddress ) != nullptr.
@Lectem Lectem force-pushed the refactor/unify-module-caches branch from 9be221a to aa234d0 Compare October 11, 2025 16:33
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp Outdated
Comment thread public/client/TracyCallstack.cpp
Comment thread public/client/TracyCallstack.cpp Outdated
const auto windirlen = strlen( windir );

const auto sz = needed / sizeof( LPVOID );
s_krnlCache = (KernelDriver*)tracy_malloc( sizeof(KernelDriver) * sz );
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The number of kernel modules is known here. I wonder if this can be preserved with the new interface?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is indeed something that bothered me a bit.

I suppose we could either delay the creation of the cache (move it out of CreateImageCaches), or create it with a capacity of 0 (which FastVector currently does not support) and then reserve the new size explicitely.

Do you want me to implement something for this ?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure if it'd be worthwhile to complicate the FastVector implementation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Well it would simply require extracting AllocMore into a SetReserve function and remove the assert( capacity != 0 ); but yeah, not sure it's worth the trouble in this specific case. It shouldn't save that much memory nor time anyway.

Comment thread public/client/TracyCallstack.cpp
Comment thread public/client/TracyCallstack.cpp
@wolfpld
Copy link
Copy Markdown
Owner

wolfpld commented Oct 23, 2025

The rest looks good.

Image cache will remain potentially unsorted until first access or `Sort` is called explicitely.
This can be CopyStringFast since we allocated the cache on the same thread.
…IMAGE_CACHE

Windows is already using a cache, and the only platforms that won't have one for now are those without dl_iterate_phdr.
- Introduce both s_imageCache and s_krnlCache on all platforms, even if unused (will be reused later to unify platforms handling)
- This means that what userland images that used to be unsorted are now sorted
@Lectem Lectem force-pushed the refactor/unify-module-caches branch from aa234d0 to 8ccc76c Compare October 24, 2025 11:34
@wolfpld wolfpld merged commit 7b8d868 into wolfpld:master Oct 24, 2025
5 of 6 checks passed
@Lectem Lectem deleted the refactor/unify-module-caches branch October 24, 2025 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants