maskit commented on code in PR #13338:
URL: https://github.com/apache/trafficserver/pull/13338#discussion_r3478940696
##########
plugins/experimental/jax_fingerprint/context_map.h:
##########
@@ -27,46 +27,63 @@
#pragma once
-#include "config.h"
#include "context.h"
-#include <string>
+#include "ts/ts.h"
+
+#include <array>
+#include <cstddef>
+#include <cstdint>
#include <string_view>
-#include <unordered_map>
-#include <version>
+#include <utility>
+
+#ifndef JAX_FINGERPRINT_MAX_METHODS
+#define JAX_FINGERPRINT_MAX_METHODS 8
+#endif
/**
* @brief Container holding JAxContext instances for multiple methods.
*
* ATS has a limited number of user arg slots (~4 per type). When loading
* many jax_fingerprint instances, we share a single slot and store all
- * contexts in this map, keyed by method name.
+ * contexts in this inline fixed-size table, keyed by method name. The
+ * table size is set at build time via JAX_FINGERPRINT_MAX_METHODS.
+ *
+ * Lookup is a linear scan over std::string_view keys (Method::name points
+ * to a string literal with static storage duration, so storing the view
+ * is safe).
*/
class ContextMap
{
public:
+ static constexpr std::size_t MAX_METHODS = JAX_FINGERPRINT_MAX_METHODS;
+ static_assert(MAX_METHODS >= 1, "Must accommodate at least one
fingerprinting method");
+
~ContextMap()
{
- for (auto &pair : m_contexts) {
- delete pair.second;
+ for (std::uint8_t i = 0; i < _size; ++i) {
Review Comment:
Fixed
##########
plugins/experimental/jax_fingerprint/context_map.h:
##########
@@ -27,46 +27,63 @@
#pragma once
-#include "config.h"
#include "context.h"
-#include <string>
+#include "ts/ts.h"
+
+#include <array>
+#include <cstddef>
+#include <cstdint>
#include <string_view>
-#include <unordered_map>
-#include <version>
+#include <utility>
+
+#ifndef JAX_FINGERPRINT_MAX_METHODS
+#define JAX_FINGERPRINT_MAX_METHODS 8
+#endif
/**
* @brief Container holding JAxContext instances for multiple methods.
*
* ATS has a limited number of user arg slots (~4 per type). When loading
* many jax_fingerprint instances, we share a single slot and store all
- * contexts in this map, keyed by method name.
+ * contexts in this inline fixed-size table, keyed by method name. The
+ * table size is set at build time via JAX_FINGERPRINT_MAX_METHODS.
+ *
+ * Lookup is a linear scan over std::string_view keys (Method::name points
+ * to a string literal with static storage duration, so storing the view
+ * is safe).
*/
class ContextMap
{
public:
+ static constexpr std::size_t MAX_METHODS = JAX_FINGERPRINT_MAX_METHODS;
+ static_assert(MAX_METHODS >= 1, "Must accommodate at least one
fingerprinting method");
+
~ContextMap()
{
- for (auto &pair : m_contexts) {
- delete pair.second;
+ for (std::uint8_t i = 0; i < _size; ++i) {
+ delete _slots[i].second;
}
}
/**
* @brief Store a context for a method.
- * @param[in] method_name The method name (e.g., "JA3", "JA4").
+ * @param[in] method_name The method name (e.g., "JA3", "JA4"). Must
reference
+ * a string with lifetime >= the ContextMap (typically a string literal).
* @param[in] ctx The context to store. Ownership is transferred.
*/
void
set(std::string_view method_name, JAxContext *ctx)
{
- auto it = find_context(method_name);
- if (it != m_contexts.end()) {
- delete it->second;
- it->second = ctx;
- } else {
- m_contexts.emplace(std::string{method_name}, ctx);
+ for (std::uint8_t i = 0; i < _size; ++i) {
Review Comment:
Fixed
##########
plugins/experimental/jax_fingerprint/context_map.h:
##########
@@ -75,10 +92,14 @@ class ContextMap
* @return The context, or nullptr if not found.
*/
JAxContext *
- get(std::string_view method_name)
+ get(std::string_view method_name) const
{
- auto it = find_context(method_name);
- return it != m_contexts.end() ? it->second : nullptr;
+ for (std::uint8_t i = 0; i < _size; ++i) {
Review Comment:
Fixed
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
