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

Separate C source into many translation units #255

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

fwilliams
Copy link
Contributor

Enables faster compilation and easier code readability

Copy link
Collaborator

@liruilong940607 liruilong940607 left a comment

Choose a reason for hiding this comment

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

LGTM! @vye16 will take a look as well.

Copy link
Collaborator

@vye16 vye16 left a comment

Choose a reason for hiding this comment

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

Hey! I support splitting these up into separate files, it'll be much easier to extend in the future. I think it would also be nice to add some header files, maybe for fully_fused_projection and rasterize_to_pixels, but that can be in the future, since we don't currently have headers for those right now. Otherwise this looks great to me!

@vye16 vye16 merged commit 2f0bb12 into nerfstudio-project:main Jul 3, 2024
2 checks passed
@yklcs
Copy link

yklcs commented Jul 4, 2024

Note that the default parallelism of Ninja uses num_cores + 2 parallel jobs which easily results in swapping/OoM.
I needed to set the MAX_JOBS environment variable to something more sane to get gsplat to compile with this PR merged.

@liruilong940607
Copy link
Collaborator

Note that the default parallelism of Ninja uses num_cores + 2 parallel jobs which easily results in swapping/OoM. I needed to set the MAX_JOBS environment variable to something more sane to get gsplat to compile with this PR merged.

Ah true. Do you know an elegant way to limit it in setup.py and JIT compiling at here?

# If the build exists, we assume the extension has been built
# and we can load it.
_C = load(
name=name,
sources=sources,
extra_cflags=extra_cflags,
extra_cuda_cflags=extra_cuda_cflags,
extra_include_paths=extra_include_paths,
)

Ideally we should set a default parallelism to a reasonable one (e.g., 12) but still allow user to overwrite that with MAX_JOBS.

cc @fwilliams

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.

4 participants