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

Optimize Quantity.parse to avoid regex overhead #4992

Merged
merged 3 commits into from
Mar 28, 2023
Merged

Optimize Quantity.parse to avoid regex overhead #4992

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
e774bb3
Optimize Quantity.parse to avoid regex overhead
schlosna Mar 23, 2023
fc5dd0d
changelog
schlosna Mar 23, 2023
115a520
Address sonar warning and test coverage
schlosna Mar 23, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#### Improvements
* Fix #4477 exposing LeaderElector.release to force an elector to give up the lease
* Fix #4992: Optimize Quantity parsing to avoid regex overhead

#### Dependency Upgrade

Expand Down
55 changes: 48 additions & 7 deletions ...nerator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/api/model/Quantity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@
})
@Buildable(editableEnabled = false, validationEnabled = false, generateBuilderPackage = true, builderPackage = "io.fabric8.kubernetes.api.builder")
public class Quantity implements Serializable {

private static final String AT_LEAST_ONE_DIGIT_REGEX = ".*\\d+.*";
private String amount;
private String format = "";
private Map<String, Object> additionalProperties = new HashMap<>();
Expand Down Expand Up @@ -148,7 +146,7 @@ public static BigDecimal getAmountInBytes(Quantity quantity) throws ArithmeticEx
Quantity amountFormatPair = parse(value);
String formatStr = amountFormatPair.getFormat();
// Handle Decimal exponent case
if ((formatStr.matches(AT_LEAST_ONE_DIGIT_REGEX)) && formatStr.length() > 1) {
if (containsAtLeastOneDigit(formatStr) && formatStr.length() > 1) {
int exponent = Integer.parseInt(formatStr.substring(1));
return new BigDecimal("10").pow(exponent, MathContext.DECIMAL64).multiply(new BigDecimal(amountFormatPair.getAmount()));
}
Expand Down Expand Up @@ -213,6 +211,19 @@ public static BigDecimal getAmountInBytes(Quantity quantity) throws ArithmeticEx
return digit.multiply(multiple);
}

/**
* @param value
* @return true if the specified value contains at least one digit, otherwise false
*/
static boolean containsAtLeastOneDigit(String value) {
for (int i = 0; i < value.length(); i++) {
if (Character.isDigit(value.charAt(i))) {
return true;
}
}
return false;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down Expand Up @@ -249,17 +260,47 @@ public static Quantity parse(String quantityAsString) {
if (quantityAsString == null || quantityAsString.isEmpty()) {
throw new IllegalArgumentException("Invalid quantity string format passed.");
}
String[] quantityComponents = quantityAsString.split("[eEinumkKMGTP]+");
String amountStr = quantityComponents[0];
String formatStr = quantityAsString.substring(quantityComponents[0].length());

int unitIndex = indexOfUnit(quantityAsString);
String amountStr = quantityAsString.substring(0, unitIndex);
String formatStr = quantityAsString.substring(unitIndex);
// For cases like 4e9 or 129e-6, formatStr would be e9 and e-6 respectively
// we need to check whether this is valid too. It must not end with character.
if (formatStr.matches(AT_LEAST_ONE_DIGIT_REGEX) && Character.isAlphabetic(formatStr.charAt(formatStr.length() - 1))) {
if (containsAtLeastOneDigit(formatStr) && Character.isAlphabetic(formatStr.charAt(formatStr.length() - 1))) {
throw new IllegalArgumentException("Invalid quantity string format passed");
}
return new Quantity(amountStr, formatStr);
}

/**
* @param quantityAsString quantity as a string
* @return the first index containing a unit character, or the length of the string if no element provided
*/
static int indexOfUnit(String quantityAsString) {
for (int i = 0; i < quantityAsString.length(); i++) {
char ch = quantityAsString.charAt(i);
switch (ch) {
case 'e':
case 'E':
case 'i':
case 'n':
case 'u':
case 'm':
case 'k':
case 'K':
case 'M':
case 'G':
case 'T':
case 'P':
return i;
default:
//noinspection UnnecessaryContinue - satisfy Sonar
continue;
Comment on lines +296 to +298
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this default case, Sonar flags this as a critical Add a default case to this switch. , though with the default case of continue IntelliJ then flags it as unnecessary. Defer to your preferences if you'd like an empty default case or if the comment suppressing IntelliJ warning is ok with you all.

Copy link
Member

Choose a reason for hiding this comment

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

Sonar false positives can be disabled by using a @SuppressWarnings annotation followed by the rule id e.g. @SuppressWarnings("java:S107")

}
}
return quantityAsString.length();
}

@JsonAnyGetter
public Map<String, Object> getAdditionalProperties() {
return this.additionalProperties;
Expand Down
34 changes: 34 additions & 0 deletions ...tor/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/api/model/QuantityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
import java.math.BigDecimal;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class QuantityTest {
private final ObjectMapper mapper = new ObjectMapper();
Expand Down Expand Up @@ -86,6 +88,7 @@ public void testNegativeExponents() {
public void testExponents() {
assertEquals("129000000", Quantity.getAmountInBytes(new Quantity("129e6")).toString());
assertEquals("129000000", Quantity.getAmountInBytes(new Quantity("129e+6")).toString());
assertEquals("1234567890", Quantity.getAmountInBytes(new Quantity("1234567890")).toString());
assertEquals("8192", Quantity.getAmountInBytes(new Quantity("8Ki")).toString());
assertEquals("7340032", Quantity.getAmountInBytes(new Quantity("7Mi")).toString());
assertEquals("6442450944", Quantity.getAmountInBytes(new Quantity("6Gi")).toString());
Expand Down Expand Up @@ -129,6 +132,7 @@ public void testEquality() {
public void testExponent() {
assertEquals("10000", Quantity.getAmountInBytes(new Quantity("1e4")).toString());
assertEquals("2000000000", Quantity.getAmountInBytes(new Quantity("2E9")).toString());
assertEquals("2000000000000", Quantity.getAmountInBytes(new Quantity("2E12")).toString());
}

@Test
Expand All @@ -148,6 +152,7 @@ public void testStringConversion() {
assertEquals("1Ki", new Quantity("1Ki").toString());
assertEquals("32Mi", new Quantity("32Mi").toString());
assertEquals("1e3", new Quantity("1e3").toString());
assertEquals("1e10", new Quantity("1e10").toString());
assertEquals("1e-3", new Quantity("1e-3").toString());
assertEquals("100k", new Quantity("100k").toString());
assertEquals("100001m", new Quantity("100001m").toString());
Expand All @@ -163,5 +168,34 @@ public void testParseFailure() {
assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity()));
assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4MiB")));
assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4megabyte")));
assertThrows(IllegalArgumentException.class, () -> Quantity.getAmountInBytes(new Quantity("4c")));
}

@Test
@DisplayName("Test containsAtLeastOneDigit method")
public void testContainsAtLeastOneDigit() {
assertTrue(Quantity.containsAtLeastOneDigit("0"));
assertTrue(Quantity.containsAtLeastOneDigit(".1"));
assertTrue(Quantity.containsAtLeastOneDigit(".1e2"));
assertTrue(Quantity.containsAtLeastOneDigit("1.2e3"));
assertTrue(Quantity.containsAtLeastOneDigit("-1K"));

assertFalse(Quantity.containsAtLeastOneDigit(""));
assertFalse(Quantity.containsAtLeastOneDigit("e"));
assertFalse(Quantity.containsAtLeastOneDigit("Mi"));
}

@Test
@DisplayName("Test indexOfUnit method")
public void testIndexOfUnit() {
assertEquals(0, Quantity.indexOfUnit(""));
assertEquals(1, Quantity.indexOfUnit("0"));
assertEquals(1, Quantity.indexOfUnit("1"));
assertEquals(3, Quantity.indexOfUnit("123"));
assertEquals(2, Quantity.indexOfUnit("12Mi"));
assertEquals(3, Quantity.indexOfUnit("123K"));
assertEquals(1, Quantity.indexOfUnit("1e3"));
assertEquals(4, Quantity.indexOfUnit("123 K"));
assertEquals(4, Quantity.indexOfUnit("123c"));
}
}