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.

Sign up for GitHub

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

fix: if the max ordinal for a map value is zero then reserve 1 bit for it instead of nothing #713

Merged
merged 3 commits into from
Jan 13, 2025
Merged

fix: if the max ordinal for a map value is zero then reserve 1 bit for it instead of nothing #713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ba691b
fix: if the max ordinal for a map value is zero then reserve 1 bit fo…
eduardoramirez Dec 19, 2024
400e645
(contd.) fix: if the max ordinal for a map value is zero then reserve…
Sunjeet Dec 20, 2024
db8e046
add consumer code to account for the bad scenario to allow to exit ou…
eduardoramirez Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapDeltaApplicator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,14 @@ public void applyDelta() {
target.bitsPerMapPointer = delta.bitsPerMapPointer;
target.bitsPerMapSizeValue = delta.bitsPerMapSizeValue;
target.bitsPerKeyElement = delta.bitsPerKeyElement;
target.bitsPerValueElement = delta.bitsPerValueElement;
// Prior to Jan 2025, the producer was able to generate blobs where it reserved 0 bits for the Map value
// element when all keys stored the same record and that record's ordinal was 0. In this case, when reading the
// Map value of the last bucket, the code would trigger ArrayIndexOutOfBoundsException since there was no space
// allocated for the value element. This is a workaround to avoid the exception when transitioning out of one
// of these bad blobs.
target.bitsPerValueElement = delta.bitsPerValueElement == 0 ? 1 : delta.bitsPerValueElement;
target.bitsPerFixedLengthMapPortion = delta.bitsPerFixedLengthMapPortion;
target.bitsPerMapEntry = delta.bitsPerMapEntry;
target.bitsPerMapEntry = target.bitsPerKeyElement + target.bitsPerValueElement;
target.emptyBucketKeyValue = delta.emptyBucketKeyValue;
target.totalNumberOfBuckets = delta.totalNumberOfBuckets;

Expand Down Expand Up @@ -143,7 +148,15 @@ private void mergeOrdinal(int ordinal) {
if(!removeData) {
for(long bucketIdx=currentFromStateStartBucket; bucketIdx<fromDataEndBucket; bucketIdx++) {
long bucketKey = from.entryData.getElementValue(bucketIdx * from.bitsPerMapEntry, from.bitsPerKeyElement);
long bucketValue = from.entryData.getElementValue(bucketIdx * from.bitsPerMapEntry + from.bitsPerKeyElement, from.bitsPerValueElement);
// Prior to Jan 2025, the producer was able to generate blobs where it reserved 0 bits for the Map value
// element when all keys stored the same record and that record's ordinal was 0. In this case, when reading the
// Map value of the last bucket, the code would trigger ArrayIndexOutOfBoundsException since there was no space
// allocated for the value element. This is a workaround to avoid the exception when transitioning out of one
// of these bad blobs.
long bucketValue =
from.bitsPerValueElement == 0
? 0
: from.entryData.getElementValue(bucketIdx * from.bitsPerMapEntry + from.bitsPerKeyElement, from.bitsPerValueElement);
if(bucketKey == from.emptyBucketKeyValue)
bucketKey = target.emptyBucketKeyValue;
long currentWriteStartBucketBit = currentWriteStartBucket * target.bitsPerMapEntry;
Expand Down
4 changes: 2 additions & 2 deletions hollow/src/main/java/com/netflix/hollow/core/write/HollowMapTypeWriteState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ private void gatherStatistics() {
}

bitsPerKeyElement = 64 - Long.numberOfLeadingZeros(maxKeyOrdinal + 1);
bitsPerValueElement = 64 - Long.numberOfLeadingZeros(maxValueOrdinal);
bitsPerValueElement = maxValueOrdinal == 0 ? 1 : 64 - Long.numberOfLeadingZeros(maxValueOrdinal);
Copy link
Contributor

@Sunjeet Sunjeet Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repro

    @Test
    public void triggerBugWithCurrentHashImpl() {
    ...    
        producer.runCycle((state) -> {
            for (int i=0; i<16384; i++) {  // exactly 16384 entries in the map, or a multiple thereof
                final long id = new Long(i);
                state.add(
                        new MapOfMovies(
                            new HashMap<Long, MyMovie>() {{
                                put(id, new MyMovie(0, "The Matrix")); // all entries pointing to same object at ordinal 0
                }}));
            }
        });
    }


bitsPerMapSizeValue = 64 - Long.numberOfLeadingZeros(maxMapSize);

Expand Down Expand Up @@ -185,7 +185,7 @@ private void calculateNumShards() {
}

long bitsPerKeyElement = 64 - Long.numberOfLeadingZeros(maxKeyOrdinal + 1);
long bitsPerValueElement = 64 - Long.numberOfLeadingZeros(maxValueOrdinal);
long bitsPerValueElement = maxValueOrdinal == 0 ? 1 : 64 - Long.numberOfLeadingZeros(maxValueOrdinal);
long bitsPerMapSizeValue = 64 - Long.numberOfLeadingZeros(maxMapSize);
long bitsPerMapPointer = 64 - Long.numberOfLeadingZeros(totalOfMapBuckets);

Expand Down
Loading