-
Notifications
You must be signed in to change notification settings - Fork 146
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
Large Volume Overflow #1639
Comments
This is actually a Realm bug. Here's an even smaller Realm-only reproducer:
Something is going wrong in the
It shouldn't be possible for Realm to get rid of the sparsity map here as the two rectangles are not touching in space, so clearly the @eddy16112 @artempriakhin @muraj: Can one of you guys take a look at this failure? |
FYI, although this is clearly a bug, there's no urgency to fix this issue from my immediate perspective. I identified a reasonable workaround in my application where the failure occurred originally while creating the reproducer I provided here. |
I identified the issue in Realm. It's actually the code that computes the volume of a rectangle that is at "fault" here: The volume of this rectangle is larger than what can be represented by a 64-bit unsigned integer ( |
Okay, so I meant to write new unit tests for |
I suspect this will actually go deeper than just fixing
So clearly we need a bigger type for representing those potential volumes, and that has to be true both inside the Perhaps for now just detecting the overflow and issuing an error will be enough to not give silently bad results. |
Here's a diff that would catch two different kinds of overflow in the volume computation:
|
@lightsighter do you want to send out the patch? I think for now it's okay to acknowledge and check-fail in There are probably multiple places where this need attention |
unless this is actually blocking @mpokorny now in some ways? |
@apryakhin, this issue isn't blocking me, I have a very reasonable workaround in my application code in place, and I don't foresee having any problems with it for a while. |
@mpokorny Do you expect to have rectangles like this often that have volumes that are bigger than |
No. Initially I chose a volume that large to be as conservative as possible in the construction of that index space, which ultimately gets intersected with another index space. But in my application the bounds on the latter index space are known to be significantly smaller -- even smaller than the range I used in the passing test case above -- so in practice a volume of |
That's actually very helpful to hear. I was pretty worried there for a moment because there's going to be a lot of code in both Legion and Realm that will need to be updated to deal with this. 😅 I think what we'll probably do is put in some detection code for now to detect these overflows as we gradually move to handling the large volume cases. For now keeping your rects under |
As discussed offline we are going to work on initial planning for a long-term fix to handle large volume sizes..across realm. This won't be P0 though at the moment. The immediate action would be to add overflow-checkers across the code base. |
Adding myself also since Legion also needs to check for large volume overflow. |
The draft is out: Once I fix CI etc...will send it out for the full review |
The following program:
Succeeds or fails depending on the values of
min
andmax
(well within the limits of the index coordinate type) provided on the command line. For exampleThe text was updated successfully, but these errors were encountered: