PengZheng commented on code in PR #470:
URL: https://github.com/apache/celix/pull/470#discussion_r1383324230
##########
libs/utils/src/properties.c:
##########
@@ -17,55 +17,87 @@
* under the License.
*/
+#include "properties.h"
+#include "celix_properties.h"
+#include "celix_properties_private.h"
+
+#include <assert.h>
+#include <errno.h>
+#include <math.h>
+#include <stdbool.h>
#include <stdio.h>
-#include <string.h>
#include <stdlib.h>
-#include <ctype.h>
-#include <stdbool.h>
+#include <string.h>
-#include "properties.h"
#include "celix_build_assert.h"
-#include "celix_properties.h"
+#include "celix_err.h"
+#include "celix_string_hash_map.h"
#include "celix_utils.h"
-#include "utils.h"
-#include "hash_map_private.h"
-#include <errno.h>
-#include "hash_map.h"
+#include "celix_stdlib_cleanup.h"
+#include "celix_convert_utils.h"
-#define MALLOC_BLOCK_SIZE 5
+#define CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE 512
+#define CELIX_SHORT_PROPERTIES_OPTIMIZATION_ENTRIES_SIZE 16
-static void parseLine(const char* line, celix_properties_t *props);
+static const char* const CELIX_PROPERTIES_BOOL_TRUE_STRVAL = "true";
+static const char* const CELIX_PROPERTIES_BOOL_FALSE_STRVAL = "false";
+static const char* const CELIX_PROPERTIES_EMPTY_STRVAL = "";
-properties_pt properties_create(void) {
- return celix_properties_create();
-}
+struct celix_properties {
+ celix_string_hash_map_t* map;
-void properties_destroy(properties_pt properties) {
- celix_properties_destroy(properties);
-}
+ /**
+ * String buffer used to store the first key/value entries,
+ * so that in many cases - for usage in service properties - additional
memory allocations are not needed.
+ *
+ * @note based on some small testing most services properties seem to max
out at 11 entries.
Review Comment:
This note should apply to `entryBuffer`?
##########
libs/utils/src/properties.c:
##########
@@ -103,12 +132,275 @@ static void updateBuffers(char **key, char ** value,
char **output, int outputPo
}
}
-static void parseLine(const char* line, celix_properties_t *props) {
- int linePos = 0;
+/**
+ * Create a new string from the provided str by either using strup or storing
the string the short properties
+ * optimization string buffer.
+ */
+char* celix_properties_createString(celix_properties_t* properties, const
char* str) {
+ if (str == NULL) {
+ return (char*)CELIX_PROPERTIES_EMPTY_STRVAL;
+ }
+ size_t len = strnlen(str, CELIX_UTILS_MAX_STRLEN) + 1;
+ size_t left = CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE -
properties->currentStringBufferIndex;
+ char* result;
+ if (len < left) {
+
memcpy(&properties->stringBuffer[properties->currentStringBufferIndex], str,
len);
+ result =
&properties->stringBuffer[properties->currentStringBufferIndex];
+ properties->currentStringBufferIndex += (int)len;
+ } else {
+ result = celix_utils_strdup(str);
+ }
+ return result;
+}
+/**
+ * Free string, but first check if it a static const char* const string or
part of the short properties
+ * optimization.
+ */
+static void celix_properties_freeString(celix_properties_t* properties, char*
str) {
+ if (str == CELIX_PROPERTIES_BOOL_TRUE_STRVAL || str ==
CELIX_PROPERTIES_BOOL_FALSE_STRVAL ||
+ str == CELIX_PROPERTIES_EMPTY_STRVAL) {
+ // str is static const char* const -> nop
+ } else if (str >= properties->stringBuffer &&
+ str < (properties->stringBuffer +
CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE)) {
+ // str is part of the properties string buffer -> nop
+ } else {
+ free(str);
+ }
+}
+
+/**
+ * Fill entry and optional use the short properties optimization string buffer.
+ */
+static celix_status_t celix_properties_fillEntry(celix_properties_t*
properties,
+ celix_properties_entry_t*
entry,
+ const char** strValue,
+ const long* longValue,
+ const double* doubleValue,
+ const bool* boolValue,
+ celix_version_t*
versionValue) {
+ char convertedValueBuffer[32];
+ if (strValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_STRING;
+ entry->value = celix_properties_createString(properties, *strValue);
+ entry->typed.strValue = entry->value;
+ } else if (longValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_LONG;
+ entry->typed.longValue = *longValue;
+ int written = snprintf(convertedValueBuffer,
sizeof(convertedValueBuffer), "%li", entry->typed.longValue);
+ if (written < 0 || written >= sizeof(convertedValueBuffer)) {
+ entry->value = celix_properties_createString(properties,
convertedValueBuffer);
+ } else {
+ char* val = NULL;
+ asprintf(&val, "%li", entry->typed.longValue);
Review Comment:
Return value of `asprintf` should allways be checked. This remark appies to
several places.
##########
libs/utils/src/properties.c:
##########
@@ -17,55 +17,87 @@
* under the License.
*/
+#include "properties.h"
+#include "celix_properties.h"
+#include "celix_properties_private.h"
+
+#include <assert.h>
+#include <errno.h>
+#include <math.h>
+#include <stdbool.h>
#include <stdio.h>
-#include <string.h>
#include <stdlib.h>
-#include <ctype.h>
-#include <stdbool.h>
+#include <string.h>
-#include "properties.h"
#include "celix_build_assert.h"
-#include "celix_properties.h"
+#include "celix_err.h"
+#include "celix_string_hash_map.h"
#include "celix_utils.h"
-#include "utils.h"
-#include "hash_map_private.h"
-#include <errno.h>
-#include "hash_map.h"
+#include "celix_stdlib_cleanup.h"
+#include "celix_convert_utils.h"
-#define MALLOC_BLOCK_SIZE 5
+#define CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE 512
Review Comment:
Short properties optimization is a good idea. I am not sure whether the size
should be set according to max values. If this is enough for 80% usages, then
I consider it a good choice. Given that celix_properties are used extensively
and for various purposes other than service metadata, 128/256 may seems less
scary.
##########
libs/utils/src/properties.c:
##########
@@ -103,12 +132,275 @@ static void updateBuffers(char **key, char ** value,
char **output, int outputPo
}
}
-static void parseLine(const char* line, celix_properties_t *props) {
- int linePos = 0;
+/**
+ * Create a new string from the provided str by either using strup or storing
the string the short properties
+ * optimization string buffer.
+ */
+char* celix_properties_createString(celix_properties_t* properties, const
char* str) {
+ if (str == NULL) {
+ return (char*)CELIX_PROPERTIES_EMPTY_STRVAL;
+ }
+ size_t len = strnlen(str, CELIX_UTILS_MAX_STRLEN) + 1;
+ size_t left = CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE -
properties->currentStringBufferIndex;
+ char* result;
+ if (len < left) {
+
memcpy(&properties->stringBuffer[properties->currentStringBufferIndex], str,
len);
+ result =
&properties->stringBuffer[properties->currentStringBufferIndex];
+ properties->currentStringBufferIndex += (int)len;
+ } else {
+ result = celix_utils_strdup(str);
+ }
+ return result;
+}
+/**
+ * Free string, but first check if it a static const char* const string or
part of the short properties
+ * optimization.
+ */
+static void celix_properties_freeString(celix_properties_t* properties, char*
str) {
+ if (str == CELIX_PROPERTIES_BOOL_TRUE_STRVAL || str ==
CELIX_PROPERTIES_BOOL_FALSE_STRVAL ||
+ str == CELIX_PROPERTIES_EMPTY_STRVAL) {
+ // str is static const char* const -> nop
+ } else if (str >= properties->stringBuffer &&
+ str < (properties->stringBuffer +
CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE)) {
+ // str is part of the properties string buffer -> nop
+ } else {
+ free(str);
+ }
+}
+
+/**
+ * Fill entry and optional use the short properties optimization string buffer.
+ */
+static celix_status_t celix_properties_fillEntry(celix_properties_t*
properties,
+ celix_properties_entry_t*
entry,
+ const char** strValue,
+ const long* longValue,
+ const double* doubleValue,
+ const bool* boolValue,
+ celix_version_t*
versionValue) {
+ char convertedValueBuffer[32];
+ if (strValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_STRING;
+ entry->value = celix_properties_createString(properties, *strValue);
+ entry->typed.strValue = entry->value;
+ } else if (longValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_LONG;
+ entry->typed.longValue = *longValue;
+ int written = snprintf(convertedValueBuffer,
sizeof(convertedValueBuffer), "%li", entry->typed.longValue);
+ if (written < 0 || written >= sizeof(convertedValueBuffer)) {
+ entry->value = celix_properties_createString(properties,
convertedValueBuffer);
+ } else {
+ char* val = NULL;
+ asprintf(&val, "%li", entry->typed.longValue);
+ entry->value = val;
+ }
+ } else if (doubleValue != NULL) {
+ entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_DOUBLE;
+ entry->typed.doubleValue = *doubleValue;
+ int written = snprintf(convertedValueBuffer,
sizeof(convertedValueBuffer), "%f", entry->typed.doubleValue);
+ if (written < 0 || written >= sizeof(convertedValueBuffer)) {
+ entry->value = celix_properties_createString(properties,
convertedValueBuffer);
+ } else {
+ char* val = NULL;
+ asprintf(&val, "%f", entry->typed.doubleValue);
Review Comment:
Retrun value should be checked.
##########
libs/utils/src/properties.c:
##########
@@ -17,55 +17,87 @@
* under the License.
*/
+#include "properties.h"
+#include "celix_properties.h"
+#include "celix_properties_private.h"
+
+#include <assert.h>
+#include <errno.h>
+#include <math.h>
+#include <stdbool.h>
#include <stdio.h>
-#include <string.h>
#include <stdlib.h>
-#include <ctype.h>
-#include <stdbool.h>
+#include <string.h>
-#include "properties.h"
#include "celix_build_assert.h"
-#include "celix_properties.h"
+#include "celix_err.h"
+#include "celix_string_hash_map.h"
#include "celix_utils.h"
-#include "utils.h"
-#include "hash_map_private.h"
-#include <errno.h>
-#include "hash_map.h"
+#include "celix_stdlib_cleanup.h"
+#include "celix_convert_utils.h"
-#define MALLOC_BLOCK_SIZE 5
+#define CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE 512
+#define CELIX_SHORT_PROPERTIES_OPTIMIZATION_ENTRIES_SIZE 16
-static void parseLine(const char* line, celix_properties_t *props);
+static const char* const CELIX_PROPERTIES_BOOL_TRUE_STRVAL = "true";
+static const char* const CELIX_PROPERTIES_BOOL_FALSE_STRVAL = "false";
+static const char* const CELIX_PROPERTIES_EMPTY_STRVAL = "";
-properties_pt properties_create(void) {
- return celix_properties_create();
-}
+struct celix_properties {
+ celix_string_hash_map_t* map;
-void properties_destroy(properties_pt properties) {
- celix_properties_destroy(properties);
-}
+ /**
+ * String buffer used to store the first key/value entries,
+ * so that in many cases - for usage in service properties - additional
memory allocations are not needed.
+ *
+ * @note based on some small testing most services properties seem to max
out at 11 entries.
+ * So 16 (next factor 2 based value) seems like a good fit.
+ */
+ char stringBuffer[CELIX_SHORT_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE];
-properties_pt properties_load(const char* filename) {
- return celix_properties_load(filename);
-}
+ /**
+ * The current string buffer index.
+ */
+ int currentStringBufferIndex;
-properties_pt properties_loadWithStream(FILE *file) {
- return celix_properties_loadWithStream(file);
-}
+ /**
+ * Entries buffer used to store the first entries, so that in many cases
additional memory allocation
+ * can be prevented.
+ *
+ * @note based on some small testing most services properties seem to max
around 300 bytes.
Review Comment:
This note should apply to `stringBuffer`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
