Copilot commented on code in PR #13338:
URL: https://github.com/apache/trafficserver/pull/13338#discussion_r3478865075
##########
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:
Using std::uint8_t for the slot index can overflow if
JAX_FINGERPRINT_MAX_METHODS (derived from ENABLE_JAX_METHODS) ever exceeds 255,
which would truncate the loop and potentially corrupt the table. Use
std::size_t for indices (and _size) to keep the container correct for any
configured method count.
##########
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:
Using std::uint8_t for the slot index can overflow if
JAX_FINGERPRINT_MAX_METHODS (derived from ENABLE_JAX_METHODS) ever exceeds 255,
which would truncate the loop and potentially corrupt the table. Use
std::size_t for indices (and _size) to keep the container correct for any
configured method count.
##########
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:
Using std::uint8_t for the slot index can overflow if
JAX_FINGERPRINT_MAX_METHODS (derived from ENABLE_JAX_METHODS) ever exceeds 255,
which would truncate the loop and potentially corrupt the table. Use
std::size_t for indices (and _size) to keep the container correct for any
configured method count.
##########
plugins/experimental/jax_fingerprint/context_map.h:
##########
@@ -88,10 +109,16 @@ class ContextMap
void
remove(std::string_view method_name)
{
- auto it = find_context(method_name);
- if (it != m_contexts.end()) {
- delete it->second;
- m_contexts.erase(it);
+ for (std::uint8_t i = 0; i < _size; ++i) {
Review Comment:
Using std::uint8_t for the slot index can overflow if
JAX_FINGERPRINT_MAX_METHODS (derived from ENABLE_JAX_METHODS) ever exceeds 255,
which would truncate the loop and potentially corrupt the table. Use
std::size_t for indices (and _size) to keep the container correct for any
configured method count.
##########
plugins/experimental/jax_fingerprint/context_map.h:
##########
@@ -102,42 +129,10 @@ class ContextMap
bool
empty() const
{
- return m_contexts.empty();
+ return _size == 0;
}
private:
- using ContextStorage = std::unordered_map<std::string, JAxContext *,
StringHash, std::equal_to<>>;
-
- /** Find context by method name with C++20 generic lookup fallback.
- *
- * C++20 generic unordered lookup allows finding with std::string_view in a
- * std::unordered_map<std::string, ...> without creating a temporary string.
- * For standard libraries without this feature, we fall back to constructing
- * a std::string for the lookup.
- *
- * @param[in] method_name The method name to look up.
- * @return Iterator to the found element, or end() if not found.
- */
- ContextStorage::iterator
- find_context(std::string_view method_name)
- {
-#ifdef __cpp_lib_generic_unordered_lookup
- return m_contexts.find(method_name);
-#else
- return m_contexts.find(std::string{method_name});
-#endif
- }
-
- /** const_iterator @overload */
- ContextStorage::const_iterator
- find_context(std::string_view method_name) const
- {
-#ifdef __cpp_lib_generic_unordered_lookup
- return m_contexts.find(method_name);
-#else
- return m_contexts.find(std::string{method_name});
-#endif
- }
-
- ContextStorage m_contexts;
+ std::array<std::pair<std::string_view, JAxContext *>, MAX_METHODS> _slots{};
+ std::uint8_t _size{0};
Review Comment:
_size is stored as std::uint8_t, but MAX_METHODS comes from
JAX_FINGERPRINT_MAX_METHODS / ENABLE_JAX_METHODS and isn't inherently limited
to 255. If a larger method list is ever configured, _size will wrap and break
bounds checks/indexing. Store _size as std::size_t to match MAX_METHODS and the
std::array index type.
--
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]
