fix interpolation behavior in various parts of the GraphQL configuration#789
fix interpolation behavior in various parts of the GraphQL configuration#789foxcool wants to merge 5 commits into
Conversation
|
Perhaps the fact that regexp began to be used requires special attention. I don’t really understand yet whether there are special overhead costs, and whether the mechanism needs to be redesigned at runtime. |
foxcool
force-pushed
the
fix/inconsistent-interpolation-behavior
branch
2 times, most recently
from
f6dffea to
0d559f6
Compare
Signed-off-by: Alexander Babenko <mail@darkfox.info>
foxcool
force-pushed
the
fix/inconsistent-interpolation-behavior
branch
from
0d559f6 to
319a166
Compare
|
Hi @foxcool, thanks for the contribution! The main problem with this approach is that regex evaluation will be performed on every request, potentially introducing a significant performance regression compared to the previous implementation. Could you provide some benchmarks comparing both versions? |
BenchmarkBodyFromParams_legacy 792 ns/op 800 B/op 12 allocs/op BenchmarkBodyFromParams_fixed 1798 ns/op 1819 B/op 25 allocs/op 2.3x slower and 2x allocations Signed-off-by: Alexander Babenko <mail@darkfox.info>
… as a reference BenchmarkInterpolation_legacy ~102 ns/op 16 B/op 1 allocs/op BenchmarkInterpolation_fixed ~437 ns/op 136 B/op 5 allocs/op (with slow regexp replacement) Signed-off-by: Alexander Babenko <mail@darkfox.info>
…without slowing down the request
Here are the benchmarks isolating just the interpolation step (no JSON marshaling):
▎ BenchmarkInterpolation_legacy ~103 ns/op 16 B/op 1 allocs/op
▎ BenchmarkInterpolation_fixed ~425 ns/op 136 B/op 5 allocs/op
The difference is expected and honest: the legacy code silently skipped compound variable strings like
"repo:{owner}/{repo} ..." entirely — so it had nothing to substitute. The fixed version correctly
processes both the single-param and compound cases, plus copies the variables map on each request. The
overhead is the cost of functionality that previously didn't work at all.
The approach mirrors config.go:665 (setup-time transformation) and proxy/request.go:GeneratePath
(per-request bytes.ReplaceAll loop), so it should be familiar.
Signed-off-by: Alexander Babenko <mail@darkfox.info>
no_params 2 ns/op 0 B/op 0 allocs ← static single_param 605 ns/op 584 B/op 9 allocs compound_params 700 ns/op 704 B/op 10 allocs ← new case mixed 1118 ns/op 920 B/op 16 allocs Signed-off-by: Alexander Babenko <mail@darkfox.info>
foxcool
force-pushed
the
fix/inconsistent-interpolation-behavior
branch
from
88157d1 to
7740661
Compare
|
Hi @thedae, thanks for the feedback! I have reworked the implementation to match the pattern used in Here are the local benchmarks no_params 2 ns/op 0 B/op 0 allocs ← static Old benchmarks and states of code given in previous commits. Local results in commit messages. |
2 participants
What does this PR do?
Fixes inconsistent interpolation behavior in various parts of the configuration bug (#660)
Also test added.