Cache config to improve latencies #348
Conversation
| b := make([]byte, 8) | ||
| binary.LittleEndian.PutUint64(b, uint64(ts)) | ||
|
|
||
| env, err := json.Marshal(os.Environ()) |
There was a problem hiding this comment.
Because env vars can not change for a running process, I think this can be left out!
| func (v *ViperProvider) hashPipelineConfig(prefix, id string, override json.RawMessage) (uint64, error) { | ||
| ts := viper.ConfigChangeAt().UnixNano() | ||
| b := make([]byte, 8) | ||
| binary.LittleEndian.PutUint64(b, uint64(ts)) |
| hashSlices = append(hashSlices, s...) | ||
| } | ||
|
|
||
| return crc64.Checksum(hashSlices, crc64.MakeTable(crc64.ECMA)), nil |
| return errors.WithStack(err) | ||
| } | ||
|
|
||
| c, ok := v.configCachge[hash] |
There was a problem hiding this comment.
We definitely need a mutex lock or we'll see panics due to concurrent access of this map!
| @@ -252,6 +305,8 @@ func (v *ViperProvider) PipelineConfig(prefix, id string, override json.RawMessa | |||
| return errors.WithStack(result.Errors()) | |||
| } | |||
|
|
|||
| v.configCachge[hash] = config | |||
There was a problem hiding this comment.
I think we should make writes expensive and reads cheap and encode the config value here to JSON and have the map as json.RawMessage. This will save one json.Encode on each read!
| c, ok := v.configCachge[hash] | ||
| if ok { | ||
| if dest != nil { | ||
| marshalled, err := json.Marshal(c) |
There was a problem hiding this comment.
Let's safe one round trip of encoding here - see line https://github.com/ory/oathkeeper/pull/348/files#diff-2449a6ea083767b149400840c05f41bdR308
I thought it might be a good idea to use glob, so I wrote a small benchmark. Unfortunately, glob seems to underperform when having to deal with interfaces (so let's stick to json), here are the results:
go test -bench . -benchmem
goos: darwin
goarch: amd64
pkg: github.com/ory/oathkeeper/docs
BenchmarkGobJSON/gob-12 7848 151588 ns/op 55343 B/op 1234 allocs/op
BenchmarkGobJSON/json-12 20253 57361 ns/op 32480 B/op 489 allocs/op
package docs
import (
"bytes"
"encoding/gob"
"encoding/json"
"io/ioutil"
"testing"
"github.com/ghodss/yaml"
"github.com/stretchr/testify/require"
)
func BenchmarkGobJSON(b *testing.B) {
gob.Register(map[string]interface{}{})
gob.Register([]interface{}{})
c, err := ioutil.ReadFile(".oathkeeper.yaml")
require.NoError(b, err)
conf, err := yaml.YAMLToJSON(c)
require.NoError(b, err)
var o map[string]interface{}
require.NoError(b, json.NewDecoder(bytes.NewBuffer(conf)).Decode(&o))
var gb bytes.Buffer
require.NoError(b, gob.NewEncoder(&gb).Encode(&o))
b.Run("gob", func(b *testing.B) {
for i := 0; i < b.N; i++ {
b.StopTimer()
m := make(map[string]interface{})
buf := bytes.NewBuffer(gb.Bytes())
b.StartTimer()
require.NoError(b, gob.NewDecoder(buf).Decode(&m))
}
})
b.Run("json", func(b *testing.B) {
for i := 0; i < b.N; i++ {
b.StopTimer()
m := make(map[string]interface{})
buf := bytes.NewBuffer(conf)
b.StartTimer()
require.NoError(b, json.NewDecoder(buf).Decode(&m))
}
})
}
| @@ -305,7 +299,13 @@ func (v *ViperProvider) PipelineConfig(prefix, id string, override json.RawMessa | |||
| return errors.WithStack(result.Errors()) | |||
| } | |||
|
|
|||
| v.configCachge[hash] = config | |||
| marshalled, err = json.Marshal(config) | |||
There was a problem hiding this comment.
marshalled should already be the proper type and payload (see https://github.com/ory/oathkeeper/pull/348/files/ede19d9b718fa4d91191ae9d138e89546305440c..8231b9aa87680589b8c42c25a46c5188acc446ca#diff-2449a6ea083767b149400840c05f41bdR296) so I don't think we need to encode it again!
There was a problem hiding this comment.
yeah :D quiet obviously... Removed the unnecessary marshall...
| require.NoError(t, os.Setenv("AUTHENTICATORS_OAUTH2_INTROSPECTION_CONFIG_INTROSPECTION_URL", "")) | ||
|
|
||
| require.NoError(t, p.PipelineConfig("authenticators", "oauth2_introspection", nil, &res)) |
There was a problem hiding this comment.
This was basically the root cause of my intention to cache the environment ;) But as you've correctly pointed out there will be no changes to the env for the running process... So I've simply adjusted the test to reflect this.
There was a problem hiding this comment.
Ohh, I see - that totally makes sense :)
| 594 20119202 ns/op | ||
|
|
||
| v0.35.2 | ||
| 3048037 3908 ns/op |
ecktom
force-pushed
the
feat/hash_config
branch
from
8231b9a to
809a1fe
Compare
|
Awesome, thank you so much for your hard work! |
Related issue
Fixes #346
Blocked by ory/viper#7
Proposed changes
Did a first draft here. Also had to hash the entire ENV in order to allow overrides which made things like 30-40% slower. ;) Any idea for that?
Guess we could tweak things even more eg.
which however would make readability worse.
Checklist
vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.