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

feat: add laz-parallel feature #70

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

tmontaigu
Copy link
Contributor

@tmontaigu tmontaigu commented Mar 26, 2024

This adds the laz-parallel feature
(which enables laz-rs if not already enabled).

When enabled, las-rs will use laz-rs parallel decompressor when reading many points at once (read_n_into, read_next_points) improving the read speed

For example, using the file linked in #64:

  • with features=laz using calls to read(), read_n_into, etc I read all the points in 28s
  • with features=laz-parallel and calls to read() I read the all the points in 28s
  • with features=laz-parallel and calls to read_n_into(),read_all_points I read the all the points in 8s

The same kind of work can be done for writing LAZ

@gadomski gadomski self-requested a review March 29, 2024 13:10
Copy link
Owner

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Makes sense to me, but can you document the feature in the README as well? Thanks!

This adds the `laz-parallel` feature
(which enables laz-rs if not already enabled).

When enabled, las-rs will use laz-rs parallel decompressor when reading
many points at once (read_n_into, read_next_points) improving the
read speed
@gadomski gadomski merged commit c55d851 into gadomski:main Apr 4, 2024
3 checks passed
@gadomski
Copy link
Owner

gadomski commented Apr 4, 2024

@tmontaigu what are the reasons someone would not want to use laz-parallel?

@tmontaigu
Copy link
Contributor Author

Apart from wanting to avoid compiling unused dependencies I don't see any

@gadomski
Copy link
Owner

gadomski commented Apr 4, 2024

Then I'm inclined to flip the flags — make laz be the parallel version, and add a laz-single-threaded (or something) to be the more verbose but smaller type. I think most folks will want parallel (who doesn't like speed?).

@tmontaigu
Copy link
Contributor Author

One slight problem with that is that the features laz and laz-single-threaded, would become mutually exclusive features, as in, if one gives both laz and laz-single-threaded feature, that laz would still work in parallel mode.

And mutually exclusive features should generally be avoided (https://doc.rust-lang.org/cargo/reference/features.html?highlight=additive#mutually-exclusive-features)

So we can either:

  1. Ignore this potential thing (and so if somehow due to the depency graph makes it so that both laz and laz-single-threaded are given, parallel mode takes priority)
  2. Only have a laz feature which always enable the parallel mode of laz (and wait for user to come in and say "yes I actually want to avoid compiling the extra dependencies needed for parallelism support)

@gadomski
Copy link
Owner

gadomski commented Apr 5, 2024

Only have a laz feature which always enable the parallel mode of laz (and wait for user to come in and say "yes I actually want to avoid compiling the extra dependencies needed for parallelism support)

I think because it's a extra feature on laz, it makes sense to keep it as a configurable option on las.

I played around with implementing this morning, and I don't love how it looks — I think I'll just keep the status quo (the way things were set up by this PR). Thanks for chatting it through and the contribution @tmontaigu ❤️ .

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.

2 participants