[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8052: Perf: Introduce TSMimeHdrFieldFast* APIs

[GitBox <gi...@apache.org>]

SolidWallOfCode commented on a change in pull request #8052:
URL: https://github.com/apache/trafficserver/pull/8052#discussion_r788079503



##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -792,6 +792,23 @@ sdk_free_field_handle(TSMBuffer bufp, MIMEFieldSDKHandle *field_handle)
   }
 }
 
+/**

Review comment:
       Really need a `static_assert` here to verify `sizeof(TSHdrHandle) == sizeof(MIMEFieldSDKHandle)`

##########
File path: include/ts/apidefs.h.in
##########
@@ -167,6 +167,15 @@ typedef struct {
   size_t data_size; ///< Amount of message data.
 } TSPluginMsg;
 
+/**
+    A struct to avoid heap allocations in TSMimeHdrField* APIs
+  */
+typedef struct {
+  uint8_t _d[24]; // private
+} TSHdrHandle;
+
+#define TS_MLOC_FROM_HDR_HANDLE(HH_) ((TSMLoc)((HH_)._d))

Review comment:
       Is this really needed? Also, no C-style casts!

##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -792,6 +792,23 @@ sdk_free_field_handle(TSMBuffer bufp, MIMEFieldSDKHandle *field_handle)
   }
 }
 
+/**
+   TSHdrHandle
+ */
+static const TSHdrHandle ts_hdr_handle_empty = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+
+static TSHdrHandle
+init_hdr_handle(MIMEHdrImpl *mh, MIMEField *field)
+{
+  MIMEFieldSDKHandle handle;
+
+  obj_init_header(&handle, HDR_HEAP_OBJ_FIELD_SDK_HANDLE, sizeof(MIMEFieldSDKHandle), 0);
+  handle.mh        = mh;
+  handle.field_ptr = field;
+
+  return *(TSHdrHandle *)&handle;

Review comment:
       That casting is obscure enough to warrant a comment.
   
   P.S. No C-style casts!

##########
File path: include/ts/apidefs.h.in
##########
@@ -167,6 +167,15 @@ typedef struct {
   size_t data_size; ///< Amount of message data.
 } TSPluginMsg;
 
+/**
+    A struct to avoid heap allocations in TSMimeHdrField* APIs
+  */
+typedef struct {
+  uint8_t _d[24]; // private

Review comment:
       I think this should be `uint64_t _d[3];` to avoid any possible alignment issues.




-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org