use flat array for field indexing instead of hashmap #247

ProBrian · 2024-09-04T08:33:10Z

To resolve KAG-5155

CLAassistant · 2024-09-04T08:33:15Z

All committers have signed the CLA.

github-actions · 2024-09-04T08:33:35Z

Luacheck Report

3 tests 3 ✅ 0s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit 0600b83.

♻️ This comment has been updated with latest results.

ProBrian · 2024-09-04T08:39:17Z

src/context.rs

@@ -25,38 +24,73 @@ impl Default for Match {
    }
 }

-pub struct Context<'a> {
-    schema: &'a Schema,


Schema is no more needed here, since Schema uses field name as hash key and Context no longer uses field name to add value, but only index in Router's fields array

ProBrian · 2024-09-04T08:42:08Z

src/context.rs

-    schema: &'a Schema,
-    values: FnvHashMap<String, Vec<Value>>,
+pub struct Context {
+    values: Vec<Option<Vec<Value>>>,


uses values array to store all field values, values index match the index of Router's fields array

ProBrian · 2024-09-04T08:48:50Z

src/router.rs

@@ -13,15 +13,17 @@ struct MatcherKey(usize, Uuid);
 pub struct Router<'a> {
    schema: &'a Schema,
    matchers: BTreeMap<MatcherKey, Expression>,
-    pub fields: HashMap<String, usize>,
+    pub fields: Vec<(String, usize)>, // fileds array of tuple(name, count)
+    pub fields_map: HashMap<String, usize>, // field name -> index map


fields is an flat array to store all field names and corresponding counts.
Here we uses an hash map fields_map to store the field's index in fields array, fields_map is not used in runtime, but only for router matcher add and delete.

ADD-SP · 2024-09-05T01:59:54Z

src/router.rs

@@ -13,15 +13,17 @@ struct MatcherKey(usize, Uuid);
 pub struct Router<'a> {
    schema: &'a Schema,
    matchers: BTreeMap<MatcherKey, Expression>,
-    pub fields: HashMap<String, usize>,
+    pub fields: Vec<(String, usize)>, // fileds array of tuple(name, count)


fileds, just a typo.

ADD-SP · 2024-09-05T02:27:03Z

lib/resty/router/context.lua

    }, _MT)

    return c
 end


-function _M:add_value(field, value)
+function _M:add_value(index, field, value)


Does this mean the caller has to maintain a map from field_name to index?

I am curious about this too. Could we use something like HashMap<String, usize> from Route to do the internal field name to index map instead of exposing the internal index to the outside world? But this sounds like making the whole improvement meaningless.

Currently, I am aware that We got three primary different components of ATC-Router, which are Route, Schema, and Context. Route is created from Schema and performs execution on Context, in a relatively loose manner, Context is related with Schema but not with Route.

Is this necessary to not create Context from Route but from nothing? The former solution will allow us to integrate the index onto Context and will allow us to use an updated Schema::fields, like Route::fields but with an index inside the map. Since the HashMap lookup is not avoided inside self.schema:get_field_type(field), I think it is better to get the index from it as well.

I do not fully understand the ATC-Router, correct me if that's impossible.

The caller uses router:get_fields to maintain an array of fields, here is the usage in kong:

function _M:fields_visitor(params, ctx, cb, cb_arg) for idx, field in ipairs(self.fields) do local value = self:get_value(field, params, ctx) -- add_value invoked in the callback local res, err = cb(field, value, cb_arg, idx) if not res then return nil, err end end -- for fields return true end

This is a loose couple between Context::values and Router::fields, only using index to match.

Is this necessary to not create Context from Route but from nothing? The former solution will allow us to integrate the index onto Context and will allow us to use an updated Schema::fields, like Route::fields but with an index inside the map. Since the HashMap lookup is not avoided inside self.schema:get_field_type(field), I think it is better to get the index from it as well.

Yes the previous Context implementation keeps Schema, which is used for type validation. But noticed that Schema::fields is different from Router::fields, Router::fields and exactly used field in configured router rules, like ['http.path', 'http.header.foo', 'http.header.bar'], but Schema::fields maintains fixed schema of field, which like: {'http.path': 'string', 'http.headers.*': 'string', 'net.src.port': 'ipaddr'}, so there are not one-one mapping between router fields and schemas.
in previous version, we keep schema in Context to do the field type validation, and for now, since we don't pass field name into Context anymore, the schema is useless now. However, We still check the field type with schema in the Lua side, before invoking add_value.

This is a loose couple between Context::values and Router::fields, only using index to match.

As the end developer, I don't want to maintain this map by myself, this should be done inside the crate.

This is a loose couple between Context::values and Router::fields, only using index to match.

As the end developer, I don't want to maintain this map by myself, this should be done inside the crate.

Sure, if we consider the ease of use, and robustness of this module, it's better to maintain the router fields inside the Context, assume that we have an fields(map of name: index) inside the context, and we provide the API to end user like add_value(field, value), then the implementation logic maybe like:

function _M:add_value(field_name, value) local index = self.fields[field_name] -- an hash find occurs here. ... clib.context_add_value(index, ...)

And what this KAG tries to do is to avoid unnecessary hash find to improve performance, which is inspired by the implementation of nginx's variable indexed fields mechanism.

if we map to nginx, the Context is basically like r->variables, which stores a flat array of variable values(without storing the variable name to value, or name to index mapping inside). what r->variables only knows, is it can use index to get/set variable from an global structure cmcf(which likes the role Router::fields).

Context and r->variables are more likely to represent some runtime state which changes frequently, we want to transfer and access them as fast as possible(and as smaller as we can). That makes them looks not so "easy to use", but I think that's a trade-off which worth it.

What if we can store a reference of used Schema::fields along with those actual index values used in Route to some field inside Context and invoke HashMap search here, to get the index and type both?

fields_visitor invokes add_value eventually which means the HashMap lookup still happens. Is there any chance to avoid exposing the index by linking the Context and Route together?

What if we can store a reference of used Schema::fields along with those actual index values used in Route to some field inside Context and invoke HashMap search here, to get the index and type both?

fields_visitor invokes add_value eventually which means the HashMap lookup still happens. Is there any chance to avoid exposing the index by linking the Context and Route together?

The previous implementation has several hashmap finds:

Context::add_value() :

find schema to validate field type by field name(which I think is unnecessary because we already check in lua side)

insert value into self.values hashmap by field name

Expression::execute():

find context.values by lhs's field name(which is the critical path that will be executed a lot)

I think our initial is to avoid such "heavy" computation as much as possible, that's why I remove hashmap in Context and replace self.values with a vector, assume that users get a fields list already, then what we can achieve is:

field name is no long needed in Context's self.values, this reduces Context's size.

insert value into self.values no longer need hash find, just vector indexing.

find value in Expression executing no long need hash find, just vector indexing.

That would bring considerable performance improvement in runtime.

However, since we remove field name in Context, we may concern that is if we are using those APIs concurrently(e.g. router fields changed after context values constructed but matching not yet executed), we may have wrong field matching(router fields index changed but context is not aware of).
If we MUST guarantee the correctness if this condition happens, maybe we should use a version in context to match router version(version in Context, Router, maybe also in Expression, that makes the version invasive to every data structure). Or we can change the Context.values to something like Vec<struct {field_name, field_index, counter}>, and making sure field name matches the lhs.name in every matching execution(however, string comparison also takes time).

find schema to validate field type by field name(which I think is unnecessary because we already check in lua side)

I believe we checked the type twice for different intents. The Lua side type check guarantees the value passed into the Rust side is always valid, and the Rust side checks for that validity.
Should we just remove the type check on the Lua side and make the Rust side return an error if the type is invalid to reduce the redundant call of the type check? I am not sure if there are some special concerns.

I do see newly added code has internal index storage with Router::fields. But since the Schema is possibly always outlived with Router, could we use string references from Schema to build the fields by linking the Schema with the Router? Since we already did link Context with Schema on the Lua side. I am not very sure about that.

The current implementation already looks good to me.

ProBrian · 2024-09-09T06:49:18Z

src/context.rs

@@ -26,37 +25,165 @@ impl Default for Match {
 }

 pub struct Context<'a> {
-    schema: &'a Schema,
-    values: FnvHashMap<String, Vec<Value>>,
+    router: &'a Router<'a>,


bind Context with Router instead, since Router can provide more precise fields validation.

dndx · 2024-10-16T03:29:55Z

@ProBrian This PR need to be rebased

ProBrian · 2024-10-30T02:58:22Z

Local benchmark result attached (MacOS M3 chip with 36GB mem), comparing with PR branch and main branch

 Running benches/build.rs (target/release/deps/build-3f0a7ade35c0c7a4)
New: Build Router            time:   [3.0939 ms 3.1021 ms 3.1114 ms]
Old: Build Router            time:   [3.1140 ms 3.1375 ms 3.1765 ms]
                        change: [+1.2814% +2.1473% +3.3212%] (p = 0.00 < 0.05)

 
     Running benches/match_mix.rs (target/release/deps/match_mix-8ebfb4b8ad1a6fd2)
New: Match                   time:   [36.678 ns 36.807 ns 36.991 ns]
Old: Match                   time:   [56.656 ns 56.897 ns 57.178 ns]
                        change: [+10.838% +11.498% +12.176%] (p = 0.00 < 0.05)

 
     Running benches/misc_match.rs (target/release/deps/misc_match-ef04ce8d11de66a9)
  New:  route match all         time:   [89.362 ns 89.910 ns 90.633 ns]
  Old: route match all         time:   [103.81 ns 104.00 ns 104.17 ns]
 
New: route mismatch all      time:   [123.28 ns 123.52 ns 123.80 ns]
Old: route mismatch all      time:   [225.65 ns 226.23 ns 226.94 ns]

 
New: route matchers batch create and delete
                        time:   [20.357 ms 20.448 ms 20.555 ms]
Old: route matchers batch create and delete
                        time:   [19.954 ms 20.100 ms 20.272 ms]

 
     Running benches/not_match_mix.rs (target/release/deps/not_match_mix-8894a45acea74fc2)
New: Doesn't Match           time:   [13.551 ms 13.624 ms 13.715 ms]
Old: Doesn't Match           time:   [21.577 ms 22.097 ms 22.635 ms]

 
     Running benches/string.rs (target/release/deps/string-4f7ea951a6e3683d)
New: Doesn't Match           time:   [10.536 ms 10.571 ms 10.615 ms]
Old: Doesn't Match           time:   [14.018 ms 14.069 ms 14.125 ms]

 
     Running benches/test.rs (target/release/deps/test-1f097a0352e64a6c)
New: Doesn't Match           time:   [35.154 ms 35.342 ms 35.552 ms]
Old: Doesn't Match           time:   [35.460 ms 35.740 ms 36.037 ms]

github-actions bot assigned ProBrian Sep 4, 2024

ProBrian commented Sep 4, 2024

View reviewed changes

ADD-SP reviewed Sep 5, 2024

View reviewed changes

ProBrian marked this pull request as draft September 9, 2024 03:50

ProBrian commented Sep 9, 2024

View reviewed changes

ProBrian requested review from dndx, ADD-SP, xianghai2, Oyami-Srk and StarlightIbuki September 10, 2024 02:00

ProBrian added 15 commits October 29, 2024 10:01

use flat array for field indexing instead of hashmap

98c9ddd

fix lua test case

decb148

format code

6c262d9

keep add_value interface

dae678a

fix test assert

bbf0a63

fix more test cases

51d548a

fix more test cases

49f3ecd

fix add value

84f1679

fix two test suite

23f5754

fix two test suite

bf64131

fix 06 test

0721a4a

fix ngx.say does not support cdata

9b478c1

fix expected result

8c3fd20

update interface description

f7187b9

rebase main branch

e3c2d31

ProBrian force-pushed the KAG-5155 branch from 6b3afe0 to e3c2d31 Compare October 29, 2024 09:05

update readme

0600b83

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use flat array for field indexing instead of hashmap #247

use flat array for field indexing instead of hashmap #247

ProBrian commented Sep 4, 2024 •

edited by jira bot

Loading

CLAassistant commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024 •

edited

Loading

ProBrian Sep 4, 2024

ProBrian Sep 4, 2024

ProBrian Sep 4, 2024 •

edited

Loading

ADD-SP Sep 5, 2024

ADD-SP Sep 5, 2024

Oyami-Srk Sep 5, 2024 •

edited

Loading

ProBrian Sep 5, 2024

ProBrian Sep 5, 2024

ADD-SP Sep 5, 2024

ProBrian Sep 5, 2024

Oyami-Srk Sep 5, 2024

ProBrian Sep 9, 2024 •

edited

Loading

Oyami-Srk Sep 9, 2024 •

edited

Loading

ProBrian Sep 9, 2024

dndx commented Oct 16, 2024

ProBrian commented Oct 30, 2024 •

edited

Loading

use flat array for field indexing instead of hashmap #247

Are you sure you want to change the base?

use flat array for field indexing instead of hashmap #247

Conversation

ProBrian commented Sep 4, 2024 • edited by jira bot Loading

CLAassistant commented Sep 4, 2024 • edited Loading

github-actions bot commented Sep 4, 2024 • edited Loading

Luacheck Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProBrian Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oyami-Srk Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProBrian Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Oyami-Srk Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dndx commented Oct 16, 2024

ProBrian commented Oct 30, 2024 • edited Loading

ProBrian commented Sep 4, 2024 •

edited by jira bot

Loading

CLAassistant commented Sep 4, 2024 •

edited

Loading

github-actions bot commented Sep 4, 2024 •

edited

Loading

ProBrian Sep 4, 2024 •

edited

Loading

Oyami-Srk Sep 5, 2024 •

edited

Loading

ProBrian Sep 9, 2024 •

edited

Loading

Oyami-Srk Sep 9, 2024 •

edited

Loading

ProBrian commented Oct 30, 2024 •

edited

Loading