Repository navigation
Add storage support for uint64 - #3961
Merged
Merged
Conversation
Comment on lines
+41
to
+44
| template <> inline constexpr DType dtype<uint64_t>{5}; | ||
| template <> inline constexpr DType dtype<bool>{6}; | ||
| template <> inline constexpr DType dtype<std::string>{7}; | ||
| template <> inline constexpr DType dtype<time_point>{8}; |
Member
There was a problem hiding this comment.
Is changing the existing constants a good idea, or is there a risk (serialized, or exchanged via Python) data from different scipp versions becomes incompatible? Why not append in one of the gaps?
Member
Author
There was a problem hiding this comment.
I don't see how two different scipp versions could interact. We do not expose the underlying int to Python and there is no serialization code in C++.
I opted for this implementation because it keeps the ints contiguous which allows the compiler to optimise checks, e.g., in is_int.
jl-wynen
marked this pull request as ready for review
August 20, 2026 11:30
SimonHeybrock
approved these changes
Aug 25, 2026
jl-wynen
force-pushed
the
support-uint64-storage
branch
from
August 25, 2026 12:21
b0ea47e to
c2a3779
Compare
jl-wynen
enabled auto-merge
August 25, 2026 12:22
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
First step towards #3928 following the plan in #3928 (comment)
This code is taken from the current
uint64branch. I included storage-related code and some basic unary operations and equality between uint64 but not between uint64 and other types.(I will benchmark compile time and binary size tomorrow.)Here are some benchmark results. Everything was recorded on my Linux laptop.
Build time
All times are for cold builds of
_scipp.so.Release
New: 2784.86s user 126.29s system 198% cpu 24:24.46 total
Old: 2687.59s user 122.85s system 198% cpu 23:32.64 total
=> new was 97s (3%) slower (user time)
Debug
New: 1894.81s user 122.15s system 392% cpu 8:33.62 total
Old: 1819.51s user 116.94s system 393% cpu 8:12.21 total
=> new was 75s (4%) slower (user time)
Binary size
The total size of all binaries combined in a release build.
New: 58_901_640B
Old: 58_146_432B
=> new is 755kB (1%) larger
I think those increases are acceptable. But I'm not super happy with slowing down releases by ~45s (2 threads) per platform.