-
Notifications
You must be signed in to change notification settings - Fork 58
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
some additional performance improvements #39
Changes from 6 commits
4b080f1
35f3ef4
efff79a
5c5a35b
73311cc
9d34ea8
ab3cb90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,8 @@ | |
import java.util.Iterator; | ||
import java.util.Map; | ||
import java.util.NavigableMap; | ||
import java.util.Objects; | ||
import java.util.Set; | ||
import java.util.TreeMap; | ||
import java.util.stream.Collectors; | ||
|
||
import static software.amazon.event.ruler.CompoundByteTransition.coalesce; | ||
|
||
|
@@ -193,7 +191,13 @@ ByteTransition getTransitionForAllBytes() { | |
* @return All transitions contained in this map. | ||
*/ | ||
Set<ByteTransition> getTransitions() { | ||
return map.values().stream().filter(Objects::nonNull).collect(Collectors.toSet()); | ||
Set<ByteTransition> result = new HashSet<>(map.values().size()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note that the Set may end up containing less elements than map.values().size(), because 1) nulls are removed, and 2) the values in the map are not unique. I suppose it's probably more efficient to oversize the Set initially though than to undersize it by going with the default constructor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes good observation, I was aware of this, I just took the size of the values as the upper bound of the set size, but perf wise its cheaper to allocate above, than allowing the set to grow |
||
for (ByteTransition transition : map.values()) { | ||
if (transition != null) { | ||
result.add(transition); | ||
} | ||
} | ||
return result; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,7 @@ | ||
package software.amazon.event.ruler; | ||
|
||
import java.nio.charset.StandardCharsets; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
/** | ||
* Represents a range of numeric values to match against. | ||
|
@@ -56,53 +54,67 @@ private Range(Range range) { | |
public static Range lessThan(final double val) { | ||
return new Range(-Constants.FIVE_BILLION, false, val, true); | ||
} | ||
|
||
public static Range lessThanOrEqualTo(final double val) { | ||
return new Range(-Constants.FIVE_BILLION, false, val, false); | ||
} | ||
|
||
public static Range greaterThan(final double val) { | ||
return new Range(val, true, Constants.FIVE_BILLION, false); | ||
} | ||
|
||
public static Range greaterThanOrEqualTo(final double val) { | ||
return new Range(val, false, Constants.FIVE_BILLION, false); | ||
} | ||
|
||
public static Range between(final double bottom, final boolean openBottom, final double top, final boolean openTop) { | ||
return new Range(bottom, openBottom, top, openTop); | ||
} | ||
|
||
private static Range deepCopy(final Range range) { return new Range(range); } | ||
private static Range deepCopy(final Range range) { | ||
return new Range(range); | ||
} | ||
|
||
/** | ||
* This is necessitated by the fact that we do range comparisons of numbers, fixed-length strings of digits, and | ||
* in the case where the numbers represent IP addresses, they are hex digits. So we need to be able to say | ||
* "for all digits between '3' and 'C'". This is for that. | ||
* | ||
* @param first Start one digit higher than this, for example '4' | ||
* @param last Stop one digit lower than this 'B' | ||
* @return The digit list, for example [ '4, '5', '6', '7', '8', '9', '9', 'A' ] (with 'B' for longDigitSequence) | ||
* @param last Stop one digit lower than this, for example 'B' | ||
* @return The digit list, for example [ '4', '5', '6', '7', '8', '9', '9', 'A' ] (with 'B' for longDigitSequence) | ||
*/ | ||
static List<Byte> digitSequence(byte first, byte last, boolean includeFirst, boolean includeLast) { | ||
assert first <= last && first <= 'F'&& first >= '0'&& last <= 'F' && last >= '0'; | ||
static byte[] digitSequence(byte first, byte last, boolean includeFirst, boolean includeLast) { | ||
assert first <= last && first <= 'F' && first >= '0' && last <= 'F'; | ||
assert !((first == last) && !includeFirst && !includeLast); | ||
|
||
final List<Byte> bytes = new ArrayList<>(); | ||
int i = 0; | ||
while (Constants.HEX_DIGITS[i] < first) { | ||
i++; | ||
} | ||
int i = getHexByteIndex(first); | ||
int j = getHexByteIndex(last); | ||
|
||
if ((!includeFirst) && (i < (Constants.HEX_DIGITS.length - 1))) { | ||
i++; | ||
} | ||
while (Constants.HEX_DIGITS[i] < last) { | ||
bytes.add(Constants.HEX_DIGITS[i++]); | ||
} | ||
|
||
if (includeLast) { | ||
bytes.add(Constants.HEX_DIGITS[i]); | ||
j++; | ||
} | ||
|
||
byte[] bytes = new byte[j - i]; | ||
|
||
System.arraycopy(Constants.HEX_DIGITS, i, bytes, 0, j - i); | ||
|
||
return bytes; | ||
} | ||
|
||
private static int getHexByteIndex(byte value) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jonessha what do you think? I feel like using the chars helps with the clarity , and then only one obscure constant is needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks good! Thank you! |
||
// ['0'-'9'] maps to [0-9] indexes | ||
if (value >= 48 && value <= 57) { | ||
return value - 48; | ||
} | ||
// ['A'-'F'] maps to [10-15] indexes | ||
return (value - 65) + 10; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to just go with -55 here? If you're trying to lay out both steps here for understandability/readability, perhaps it would be better to go further and turn these numbers into private static final class variables? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mostly to try to keep it easy to understand, if we trust the compiler this is being optimized away. I think the variable may be the better option then |
||
} | ||
|
||
@Override | ||
public Object clone() { | ||
super.clone(); | ||
|
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.
super-duper nitpicking, but I think you can have a consistent style here and in
ByteMap.getTransitions()
if you forx.forEach( elem -> addWhenNotNull (...) )
. I'm not sure on the performance implications yet but maybe slightly readable. Maybe...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 did update this one to the new style that I believe is easier to understand.