behavior of rolling_hash2_run is ill defined #56

New issue

Open

opened 2020-09-29 19:18:18 -05:00 by lasarohedvig · 3 comments

lasarohedvig commented

2020-09-29 19:18:18 -05:00

(Migrated from github.com)

Hi, all.
Could you help me clarify a few details regarding the use of rolling_hash2_run function?

What is the value of the offset if the function returns FINGERPRINT_RET_MAX? The documentation says it is set only in case of a HIT, but the example chunking code ignores the result of the function, so I need to assume that offset is the same as if a chunk boundary had been found at the last byte of the buffer, that is, it points to the last byte of the buffer + 1. Is this correct?
What does it mean for rolling_hash2_run to return FINGERPRINT_RET_OTHER? Why does the example does not consider this return?
Do I need to call rolling_hash2_reset before every rolling_hash2_run call? Or can I use the following flow?

reset()
1: result = run()
if (result == FINGERPRINT_RET_HIT)
     process_chunk
else if (result == FINGERPRINT_RET_MAX)
     replace buffer
     goto 1

Regards

Hi, all. Could you help me clarify a few details regarding the use of `rolling_hash2_run` function? - What is the value of the offset if the function returns `FINGERPRINT_RET_MAX`? The documentation says it is set only in case of a `HIT`, but the example chunking code ignores the result of the function, so I need to assume that offset is the same as if a chunk boundary had been found at the last byte of the buffer, that is, it points to the last byte of the buffer + 1. Is this correct? - What does it mean for `rolling_hash2_run` to return `FINGERPRINT_RET_OTHER`? Why does the example does not consider this return? - Do I need to call `rolling_hash2_reset` before every `rolling_hash2_run` call? Or can I use the following flow? ``` reset() 1: result = run() if (result == FINGERPRINT_RET_HIT) process_chunk else if (result == FINGERPRINT_RET_MAX) replace buffer goto 1 ``` Regards

gbtucker commented

2020-09-29 20:25:02 -05:00

(Migrated from github.com)

Are you referring to the example at rolling_hash/chunking_with_mb_hash.c? This example does assume that there is a minimum chunk size. You don't have to call rolling_hash2_reset() but the example assumes that the chunk minimum is larger than the window size so that any computation in finding matches up the the minimum would be waisted. For this reason it skips to the min size offset and calls _reset() as this should be less work if min > window width.

The value FINGERPRINT_RET_OTHER is reserved for error conditions but so far there are no internal conditions that will set this.

I think you have the definition correct. On hit, offset is set to the position that causes the match, or ptr + 1 so the next offset to check, and it can be re-run without incrementing. On max_len, it will return the max offset - 1.

Are you referring to the example at `rolling_hash/chunking_with_mb_hash.c`? This example does assume that there is a minimum chunk size. You don't have to call `rolling_hash2_reset()` but the example assumes that the chunk minimum is larger than the window size so that any computation in finding matches up the the minimum would be waisted. For this reason it skips to the min size offset and calls _reset() as this should be less work if min > window width. The value `FINGERPRINT_RET_OTHER` is reserved for error conditions but so far there are no internal conditions that will set this. I think you have the definition correct. On hit, offset is set to the position that causes the match, or ptr + 1 so the next offset to check, and it can be re-run without incrementing. On max_len, it will return the max offset - 1.

lasarohedvig commented

2020-09-30 08:55:28 -05:00

(Migrated from github.com)

Thanks for clarifying that reset() is just a shortcut to skip bytes, that a return of MAX does set the offset and that OTHER is unused for now. I believe this should be put into the documentation.

Thanks for clarifying that `reset()` is just a shortcut to skip bytes, that a return of `MAX` does set the offset and that `OTHER` is unused for now. I believe this should be put into the documentation.

gbtucker commented

2020-09-30 17:13:18 -05:00

(Migrated from github.com)