-
Notifications
You must be signed in to change notification settings - Fork 24
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
Avoid allocating spire uint objects during apply agglomerate #6532
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jstriebel
approved these changes
Oct 5, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, mostly LGTM, just a comment about the optimizations would be nice. I tested the behavior on the deployed dev-instance, and skimmed through the code, not checking everything in detail. Do you think that's fine?
...ossos-datastore/app/com/scalableminds/webknossos/datastore/services/AgglomerateService.scala
Show resolved
Hide resolved
hotzenklotz
added a commit
that referenced
this pull request
Oct 13, 2022
…jects-created * 'master' of github.com:scalableminds/webknossos: (337 commits) Fix docs for the annotation download file format (#6546) Added total runtime information to VX reports (#6543) fix VX report for completed + skipped tasks (#6540) Avoid allocating spire uint objects during apply agglomerate (#6532) Explore remote N5 datasets (#6520) Fix MeshChunk byteOffset (Long, not Int) (#6536) update browserslist (#6505) Support new Mesh File (v3) (#6491) makes workflow_yamlContent optional (#6518) Always return 404 for Failures in Zarr Streaming (#6515) Poll wk version to notify during upgrade (#6451) add script which extracts newest changelog and creates GH release for it (#6504) release 22.10.0 (#6500) voxel³ -> voxel (#6501) Allow task type summary to identify task type when creating tasks in bulk (#6486) Fix sql evolution 090 (defer not null constraint) (#6498) SQL schema cleanup (#6492) Fix validation of layer selection when trying to start globalization of floodfills (#6497) Add "shift + w" shortcut to cycle backwards through tools (#6493) Fix filtering for public datasets in dataset table (#6496) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When applying an agglomerate mapping, each value of the segmentation data needs to be converted to Long to then look up the agglomerate id in the hdf5 array. Since the jvm does not support unsigned types, this was previously done by wrapping each signed value in a spire unsigned integer object, and then calling toLong.
This PR changes the code to skip using the spire classes and directly calling the to-long method (which performs bitwise and to do the unsigned logic) on each value.
It also moves the filter-zero logic out of the data conversion since this had a performance cost even if the filterZero boolean was false.
It also adds a pre-allocated LongBuffer instead of using
array.map(_.toLong)
, which is slightly faster, I guess the old version did not know the final length of the output array before.My measurements show that the conversion to Long is about six times faster with this PR, almost halving the time to apply an agglomerate mapping on my test data.
cc @youri-k
URL of deployed dev instance (used for testing):
Steps to test: