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.

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

fix: Fix testrender GPU regression with bad destruction order #1814

Merged
merged 2 commits into from
May 8, 2024

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented May 8, 2024

A few months back, PR #1733 seems to have switched the order that testrender destroys the shading system versus the renderer (services).

This made some subtle bugs that were only symptomatic for GPU renders, but it's because of the destructor order, where the shadingsystem's dtr still references the renderer, which cannot be destroyed yet.

The clue is that the SS's constructor takes the RS pointer as an argument. The RS, then, must have been constructed before the SS, and therefore we should expect it to be a requirement for the RS to outlast the lifetime of the SS. (Complex objects should be destroyed in the opposite order that they were constructed, if they contain references to each other.)

@lgritz lgritz requested review from chellmuth and brechtvl May 8, 2024 18:10
A few months back, PR 1733 seems to have switched the order that
testrender destroys the shading system versus the renderer (services).

This made some subtle bugs that were only symptomatic for GPU renders,
but it's because of the destructor order, where the shadingsystem's
dtr still references the renderer, which cannot be destroyed yet.

The clue is that the SS's constructor takes the RS pointer as an
argument. The RS, then, must have been constructed before the SS, and
therefore we should expect it to be a requirement for the RS to
outlast the lifetime of the SS. (Complex objects should be destroyed
in the opposite order that they were constructed, if they contain
references to each other.)

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@brechtvl
Copy link
Contributor

brechtvl commented May 8, 2024

I guess this will fix the ASAN error that came back, matching what optixraytracer already does.

diff --git a/src/testrender/simpleraytracer.cpp b/src/testrender/simpleraytracer.cpp
index d9ae6806..3607c4cc 100644
--- a/src/testrender/simpleraytracer.cpp
+++ b/src/testrender/simpleraytracer.cpp
@@ -1100,5 +1100,10 @@ SimpleRaytracer::render(int xres, int yres)
 }
 
 
+void
+SimpleRaytracer::clear()
+{
+  shaders().clear();
+}
 
 OSL_NAMESPACE_EXIT
diff --git a/src/testrender/simpleraytracer.h b/src/testrender/simpleraytracer.h
index fbddf68c..e9cae486 100644
--- a/src/testrender/simpleraytracer.h
+++ b/src/testrender/simpleraytracer.h
@@ -82,7 +82,7 @@ public:
     virtual void prepare_render();
     virtual void warmup() {}
     virtual void render(int xres, int yres);
-    virtual void clear() {}
+    virtual void clear();
 
     // After render, get the pixels into pixelbuf, if they aren't already.
     virtual void finalize_pixel_buffer() {}

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz lgritz merged commit f6b88b9 into AcademySoftwareFoundation:main May 8, 2024
25 checks passed
@lgritz lgritz deleted the lg-testrender branch May 11, 2024 04:11
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