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

Changes in film crop_size, crop_offset results in new kernel being generated every iteration #908

Open
KoykL opened this issue Sep 18, 2023 · 2 comments · May be fixed by #920
Open

Changes in film crop_size, crop_offset results in new kernel being generated every iteration #908

KoykL opened this issue Sep 18, 2023 · 2 comments · May be fixed by #920

Comments

@KoykL
Copy link

KoykL commented Sep 18, 2023

Summary

With perspective sensor, if crop_size, crop_offset of film changes every iteration, new kernel will be generated every iteration, which results in bad performance especially if pytorch inter-op is used inside rendering code.

System configuration

System information:

  OS: Red Hat Enterprise Linux release 8.8 (Ootpa)
  CPU: AMD EPYC 7313 16-Core Processor
  GPU: NVIDIA RTX A6000
  Python: 3.10.10 (main, Feb 28 2023, 09:55:02) [GCC 8.5.0 20210514 (Red Hat 8.5.0-16)]
  NVidia driver: 525.116.04
  CUDA: 11.7.64
  LLVM: -1.-1.-1

  Dr.Jit: 0.4.2
  Mitsuba: 3.3.0
     Is custom build? False
     Compiled with: GNU 10.2.1
     Variants:
        scalar_rgb
        scalar_spectral
        cuda_ad_rgb
        llvm_ad_rgb

Description

After some investigation, I realize film size, crop_size and crop_offset parameters are mi.Scalar*, and cannot be made opaque by dr.make_opaque. While perspective sensor almost made all of its parameters depending on film parameters opaque, scaled_principal_point_offset depends on film parameters, and is dynamically created in its sample_* methods.

I am considering modifying scaled_principal_point_offset into an instance variable of m_scaled_principal_point_offset, make it opaque, and update all relevant code. Would this be a reasonable fix? EDIT: this does not seem to eliminate new kernel generation.

Since such fix involves changes to official plugins, I decide to open an issue for that.

Steps to reproduce

  1. With a rendering procedure that depends on pytorch code, in a loop, call mi.render different crop_size and crop_offset film parameters in every iteration. When set drjit log to debug mode, observe new kernel being generated whenever pytorch code is called.
  2. Keep crop_size and crop_offset film parameters same in every iteration. Observe time per iteration is much faster and no new kernel being generated on every iteration.
@KoykL KoykL changed the title film crop_size, crop_offset changes results in new kernel being generated every iteration Changes in film crop_size, crop_offset results in new kernel being generated every iteration Sep 18, 2023
KoykL added a commit to KoykL/mitsuba3 that referenced this issue Sep 19, 2023
…vent new drjit kernel from being generated on every iteration when film parameters are changed; fix mitsuba-renderer#908
@njroussel
Copy link
Member

Hi @KoykL

Thanks for the bug report.

Indeed, it seems that this is not properly supported.

I am considering modifying scaled_principal_point_offset into an instance variable of m_scaled_principal_point_offset, make it opaque, and update all relevant code. Would this be a reasonable fix ?

This would not be enough, I believe there are quite a few other places that would need to change. Namely some of the internals in Integrator::render and the ImageBlock.

I will have a look at this soon.

@KoykL
Copy link
Author

KoykL commented Sep 26, 2023

Hi @njroussel,

Thank you for the help!

I previously investigated a bit but I decide to take alternative approach of using custom sensor to keep my current project going. I have summarized my previous effort in PR #920. Hopefully it will be useful.

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 a pull request may close this issue.

2 participants