-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Memop fix related to scanning for missing values #5300
Conversation
@@ -892,7 +892,7 @@ SECOND_PASS: | |||
else if (!HasNoMissingValues()) | |||
{ | |||
// Have we overwritten all the missing values? | |||
if (!ScanForMissingValues<T>(0, startOffset)) | |||
if (!ScanForMissingValues<T>(0, startOffset) && !ScanForMissingValues<T>(startOffset + length, current->length)) |
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.
Why not ScanForMissingValues(0, current->length)?
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.
Because indices from startOffset -> startOffset + length - 1
would be overwritten by the memcopy/memset and we would have given up on memop if the source had missing values in the range being copied/set.
In reply to: 194876775 [](ancestors = 194876775)
FloatArr0[i] = IntArr0[i]; | ||
} | ||
GiantPrintArray.push(FloatArr0); | ||
for (var v2 = 0; 0 < GiantPrintArray; 0) { |
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.
Is this loop needed ?
So this is because of the change in prepare segment where we were calling ScanForMissingValues too much right ? |
Right. This change does not reintroduce that regression. That's because the scan for missing values is done for different reasons in the code that that I touched in https://github.com/Microsoft/ChakraCore/pull/5110/files#diff-ebffc7a76b95c74f4a965f12627553a3R977 and this change. This PR - scan for missing values is done on an array that does not have HasNoMissingValues set and whose head segment's missing values may get overwritten by the memop. #5110 - code was trying to make sure that there were no missing values in the head segment of an array which had HasNoMissingValues set on it. If there were any missing values, we'd set the flag to false (This, I think was done to protect against situations when due to some earlier condition in PrepareSegmentForMemop the HasNoMissingValues flag was set on the array, albeit incorrectly. This scan may actually be unnecessary after the fix in this PR). |
Yeah, we would have to double check, the second scan of the end segment is probably not needed anymore. |
… missing values Merge pull request #5300 from rajatd:prepareSeg If the entire range of a memop operation fits in the head segment, properly check if there were any missing items in the head outside of that range, before setting HasNoMissingValues to true on the dst array. This is needed only after #5110.
If the entire range of a memop operation fits in the head segment, properly check if there were any missing items in the head outside of that range, before setting HasNoMissingValues to true on the dst array.
This is needed only after #5110.