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.

Sign up for GitHub

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

Jump to bottom

Frozen ThreadState Fixes #115

Closed
wants to merge 8 commits into from
Closed

Frozen ThreadState Fixes #115

wants to merge 8 commits into from

Conversation

DoeringChristian
Copy link
Contributor

This PR adds various fixes to the Frozen ThreadState #107 PR, that appeared after merging.

  • Adds logging function for capture_call_offset function
  • Fix memory leak in vcall tests by freeing the copied OptiX SBT and changing borrow to steal in test
  • Adds the ability to traverse the whole registry without a variant string

merlinND
merlinND reviewed Jan 3, 2025
View reviewed changes
Copy link
Member

@merlinND merlinND left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Small comments, overall looks fine to me!

PS: also we need to make sure that CI passes. I think that you were added to approved contributors, so probably CI will be triggered the next time you push without requiring approval?

include/drjit-core/jit.h Outdated
@@ -495,13 +495,17 @@ extern JIT_EXPORT uint32_t jit_registry_id(const void *ptr);

/// Return the largest instance ID for the given domain
/// If the \c domain is \c nullptr, it returns the number of active entries in
/// all domains for the given variant.
/// all domains for the given variant. If the \c variant is empty as well, it
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// all domains for the given variant. If the \c variant is empty as well, it
/// all domains for the given variant. If the \c variant is \c nullptr as well, it

Need to be precise since "" and nullptr are not the same thing.

include/drjit-core/jit.h
@@ -495,13 +495,17 @@ extern JIT_EXPORT uint32_t jit_registry_id(const void *ptr);

/// Return the largest instance ID for the given domain
/// If the \c domain is \c nullptr, it returns the number of active entries in
/// all domains for the given variant.
/// all domains for the given variant. If the \c variant is empty as well, it
/// traverses all registry entries.
extern JIT_EXPORT uint32_t jit_registry_id_bound(const char *variant,
const char *domain);

/// Fills the \c dest pointer array with all pointers registered in the registry.
/// \c dest must point to an array with \c jit_registry_id_bound(variant, nullptr) entries.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// \c dest must point to an array with \c jit_registry_id_bound(variant, nullptr) entries.
/// \c dest must point to an array with \c jit_registry_id_bound(variant, domain, nullptr) entries.

include/drjit-core/jit.h Outdated
extern JIT_EXPORT uint32_t jit_registry_id_bound(const char *variant,
const char *domain);

/// Fills the \c dest pointer array with all pointers registered in the registry.
/// \c dest must point to an array with \c jit_registry_id_bound(variant, nullptr) entries.
extern JIT_EXPORT void jit_registry_get_pointers(const char *variant, void **dest);
/// If variant is \c nullptr, it traverses all registry entries.
Copy link
Member

Choose a reason for hiding this comment

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

Specify whether domain can be nullptr or not.

include/drjit-core/jit.h
extern JIT_EXPORT void jit_registry_get_pointers(const char *variant, void **dest);
/// If variant is \c nullptr, it traverses all registry entries.
extern JIT_EXPORT void jit_registry_get_pointers(const char *variant,
const char *domain_name,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const char *domain_name,
const char *domain,

For consistency with the other functions.

src/record_ts.cpp
Comment on lines +2156 to +2166
for (Operation &op : recording->operations){
if (op.uses_optix){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (Operation &op : recording->operations){
if (op.uses_optix){
for (Operation &op : recording->operations) {
if (op.uses_optix) {

src/record_ts.cpp
Comment on lines +2156 to +2166
for (Operation &op : recording->operations){
if (op.uses_optix){
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that op.uses_optix is true but op.sbt is nullptr / not fully initialized? (e.g. in the case of some error).
Maybe better to check op.uses_optix && op.sbt to be safe?

src/registry.cpp Outdated
@@ -173,12 +173,15 @@ uint32_t jitc_registry_id_bound(const char *variant, const char *domain) {
return r.domains[it->second].id_bound;
}

void jitc_registry_get_pointers(const char *variant, void **dest) {
void jitc_registry_get_pointers(const char *variant, const char *domain_name,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void jitc_registry_get_pointers(const char *variant, const char *domain_name,
void jitc_registry_get_pointers(const char *variant, const char *domain,

src/registry.h Outdated
@@ -23,13 +23,16 @@ extern uint32_t jitc_registry_id(const void *ptr);

/// Return the largest instance ID for the given domain
/// If the \c domain is \c nullptr, it returns the number of active entries in
/// all domains for the given variant.
/// all domains for the given variant. If the \c variant is empty as well, it
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as in jit.h for this file.

src/registry.cpp Outdated
Comment on lines 182 to 184
if (variant == nullptr ||
(strcmp(domain.variant, variant) == 0 &&
(domain_name == nullptr || strcmp(domain.name, domain_name) == 0)))
Copy link
Member

Choose a reason for hiding this comment

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

This logic might be counter-intuitive, once would probably expect the following:

  1. variant == nullptr & domain == nullptr: all entries
  2. variant != nullptr & domain == nullptr: all entries with this variant (regardless of domain)
  3. variant == nullptr & domain != nullptr: all entries with this domain (regardless of variant)

If you think this is not desired, then the current behavior should be pointed out very explicitly in the docstring.

@DoeringChristian DoeringChristian force-pushed the variant-registry-traversal branch 2 times, most recently from 208dcc9 to 45a455f Compare January 30, 2025 13:55
@DoeringChristian DoeringChristian force-pushed the variant-registry-traversal branch from 45a455f to 7f165ed Compare January 31, 2025 09:52
@DoeringChristian DoeringChristian mentioned this pull request Feb 3, 2025
Merged
@DoeringChristian
Copy link
Contributor Author

This PR is replaced by #121, which changes the source branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlinND merlinND merlinND left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DoeringChristian @merlinND