-
Notifications
You must be signed in to change notification settings - Fork 59
PROTON-2178 Implement efficient search of encoded Symbol on ReadableBuffer #36
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
Conversation
…uffer The search algorithm chosen is Knuth-Morris-Pratt and could make use of an additional jump table with certain Symbols
|
Some benchmark results are on netty/netty#9955 |
|
I see that I'm not accounting for remaining space to search in to be less then the needle to search for :) |
gemmellr
left a comment
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.
I dont think it really makes sense to add this to the Symbol object itself, since it doesn't actually identify an encoded Symbol in the provided buffer, but rather a sequence of bytes in the buffer that matches its ascii content, but which could be from [part of] absolutely any type or value. Essentially its a general search of the buffer, which seems more suited to being outside the Symbol class if anywhere to me.
| int j = 0; | ||
| final int[] jumpTable = racyGerOrCreateJumpTable(); | ||
| final byte[] needle = _underlyingBytes; | ||
| final long length = to - from; |
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.
Given the other checking above, should it perhaps also validate that to <= from?
| throw new IllegalArgumentException("range indexes cannot be negative!"); | ||
| } | ||
| int j = 0; | ||
| final int[] jumpTable = racyGerOrCreateJumpTable(); |
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.
Not sure what Ger is ;)
| final int[] jumpTable = racyGerOrCreateJumpTable(); | ||
| final byte[] needle = _underlyingBytes; | ||
| final long length = to - from; | ||
| final ReadableBuffer haystack = buffer; |
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.
Any reason not to just declare the method arg with the desired name + final?
| final long length = to - from; | ||
| final ReadableBuffer haystack = buffer; | ||
| final int needleLength = needle.length; | ||
| for (int i = 0; i < length; i++) |
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.
Would initialising i to 'from' rather than 0 simplify things? (Plus other adjustments accordingly, e.g rename i to index, to rather than length)
|
Hi @gemmellr I can close this and sorry to have made you to review this: I've included this general purpose search method directly on AMQ code base, but still, would be useful (maybe in a new codec) a method to perform a "real" search for a specific property key in some section of AMQP, instead of a greedy approach like this. |
The search algorithm chosen is Knuth-Morris-Pratt and could make
use of an additional jump table with certain Symbols