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.
Why not ScanForMissingValues(0, current->length)?
There was a problem hiding this comment.
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) { |
|
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.