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

C++ universal "maybeowned" adapting container used everywhere instead of std::vector #3794

Closed
2 tasks done
Tracked by #4260
Wumpf opened this issue Oct 11, 2023 · 4 comments
Closed
2 tasks done
Tracked by #4260
Assignees
Labels
🌊 C++ API C/C++ API specific enhancement New feature or request
Milestone

Comments

@Wumpf
Copy link
Member

Wumpf commented Oct 11, 2023

The ComponentBatch class introduced in #3791 is not actually specific to components at all. We should build it out to a "universal" container that we use in all codegen wherever we use std::vector right now.
It is also what all serialize methods should work with.

@Wumpf Wumpf added enhancement New feature or request 🌊 C++ API C/C++ API specific labels Oct 11, 2023
Wumpf added a commit that referenced this issue Oct 11, 2023
@Wumpf
Copy link
Member Author

Wumpf commented Oct 12, 2023

When looking into this we should keep in mind TensorBuffer which might be a bit harder to solve but should also use the maybe-owned concept!

@emilk
Copy link
Member

emilk commented Oct 23, 2023

Next step of this is also to support strides

@Wumpf
Copy link
Member Author

Wumpf commented Nov 10, 2023

Talking to a user, not only strides would be useful but also byte offsets. Example: Struct like this:

struct PointAndColor { float x,y,z; uint32_t color; }
https://github.com/adujardin/zed-rerun-io/blob/dfaa374e900ff60720060759f7018e8dcdd9ae90/rerun_depth_sensing/src/main.cpp#L100

We want it to be convenient to log arrays of this struct both as points and colors!

@Wumpf Wumpf self-assigned this Nov 13, 2023
Wumpf added a commit that referenced this issue Nov 15, 2023
…ted constructs) (#4236)

### What

Not much else has changed yet except some doc massaging. First step
towards solving

* #3794

`rerun::Collection` will evolve from here on and be the corner piece of
our zero copy & component adaptability strategy (just like
ComponentBatch is already!)


_This was originally opened on the wrong branch by accident:_
*  #4225

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/4225) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4225)
- [Docs
preview](https://rerun.io/preview/3de46c8a5d294eb5ad68e5beb35a64d0610e992a/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/3de46c8a5d294eb5ad68e5beb35a64d0610e992a/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
Wumpf added a commit that referenced this issue Nov 17, 2023
…re (#4247)

* Part of the series of #3794 

### What
... and various improvements to `rerun::Collection` in general to be up
for the task. Added copy assignment/construction, added vector
conversion, removed serialization method (handled at callsites now)

Also, I overhauled and generalized our builtin
`rerun::CollectionAdapters` further. They give now more exact guarantees
on when they copy & move data.
To support the considerably increased significance and abilitites of
this data structure I added a lot more tests, in particular checking on
the aforementioned move/copy guarantees.

Huge thanks to @AnkeAnke for pointing out to me how we can support std
compatible contains generically in a trivial way
(`rerun::type_traits::value_type_t`!). This made it possible that this
PR requires almost no changes in user/test/example code since most types
just snap in the same as before.

As expected this change improves the performance on the image logging
benchmark (which logs 4 16k rgba images).
Timing on M1 Max went from ~2.9s to ~2.4s and on the AMD windows box I
use from ~3.0s to ~2.5s
(did a bunch of runs, values are excluding prepare and process setup,
AMD fluctuates a lot)


DRAFT until I'm sure this works on more compilers.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/4247) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4247)
- [Docs
preview](https://rerun.io/preview/825bf7905ccaef0f23d87a4ffe0ae406018edffa/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/825bf7905ccaef0f23d87a4ffe0ae406018edffa/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@Wumpf
Copy link
Member Author

Wumpf commented Nov 17, 2023

The original issue was solved. Split all the other things that came up into different issues and list them in a tracking issue here

@Wumpf Wumpf closed this as completed Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌊 C++ API C/C++ API specific enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants