Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse parameters in URL at validation time #149

Conversation

emnnqqs
Copy link
Contributor

@emnnqqs emnnqqs commented Nov 29, 2019

There may have some parameters define in 'url' of servers definition
and those parameter names will be need to store parameter values. In
current implementation we will parse 'url' to get parameters name for
each incoming request, now change to parse parameters in 'url' at spec
definition validation time and store results to avoid parse it for
each incoming request.

There may have some parameters define in 'url' of servers definition
and those parameter names will be need to store parameter values. In
current implementation we will parse 'url' to get parameters name for
each incoming request, now change to parse parameters in 'url' at spec
definition validation time and store results to avoid parse it for
each incoming request.
@fenollp
Copy link
Collaborator

fenollp commented Dec 1, 2019

This sounds like you want memoization. How about just defining a private map that serves as a cache for calls to openapi3.Server.ParameterNames()?

Is this for optimization reasons or is this about validating spec data before actually needing the spec to be valid? In the case of the latter I think the parsing/check should just be added to func (server *Server) Validate(c context.Context) (err error).

@emnnqqs
Copy link
Contributor Author

emnnqqs commented Dec 2, 2019

This sounds like you want memoization. How about just defining a private map that serves as a cache for calls to openapi3.Server.ParameterNames()?

Is this for optimization reasons or is this about validating spec data before actually needing the spec to be valid? In the case of the latter I think the parsing/check should just be added to func (server *Server) Validate(c context.Context) (err error).

This is for optimization reason. The purpose is to reduce cost during request validation. My first idea is also add a private map to cache previous calculated results, but map operation also more expensive than a local variable for each server:
I have a benchmark test as below:

BenchmarkServerParamNames_WithPrivateMapCache-8   	146136141	         8.20 ns/op	       0 B/op	       0 allocs/op
BenchmarkServerParamNames_CalculateEachTime-8     	 8996028	       136 ns/op	      48 B/op	       2 allocs/op
BenchmarkServerParamNames_LocalVariable-8         	1000000000	         0.283 ns/op	       0 B/op	       0 allocs/op

Benchmark codes as below:

var pmap map[string][]string = make(map[string][]string)

func (server Server) ParameterNames() ([]string, error) {
	pattern := server.URL
	if v, ok := pmap[pattern]; ok {
		return v, nil
	}
	var params []string
	for len(pattern) > 0 {
		i := strings.IndexByte(pattern, '{')
		if i < 0 {
			break
		}
		pattern = pattern[i+1:]
		i = strings.IndexByte(pattern, '}')
		if i < 0 {
			return nil, errors.New("Missing '}'")
		}
		params = append(params, strings.TrimSpace(pattern[:i]))
		pattern = pattern[i+1:]
	}
	pmap[server.URL] = params
	return params, nil
}

func (server Server) ParameterNames2() ([]string, error) {
	pattern := server.URL
	var params []string
	for len(pattern) > 0 {
		i := strings.IndexByte(pattern, '{')
		if i < 0 {
			break
		}
		pattern = pattern[i+1:]
		i = strings.IndexByte(pattern, '}')
		if i < 0 {
			return nil, errors.New("Missing '}'")
		}
		params = append(params, strings.TrimSpace(pattern[:i]))
		pattern = pattern[i+1:]
	}
	return params, nil
}

func BenchmarkServerParamNames_WithPrivateMapCache(t *testing.B) {
	server := &openapi3.Server{
		URL: "http://{x}.{y}.example.com",
	}
	server.ParameterNames()
	t.ResetTimer()
	for i := 0; i < t.N; i++ {
		p, _ := server.ParameterNames()
		if len(p) != 2 {
			panic("error")
		}
	}
}

func BenchmarkServerParamNames_CalculateEachTime(t *testing.B) {
	server := &openapi3.Server{
		URL: "http://{x}.{y}.example.com",
	}
	t.ResetTimer()
	for i := 0; i < t.N; i++ {

		p, _ := server.ParameterNames2()
		if len(p) != 2 {
			panic("error")
		}
	}
}

func BenchmarkServerParamNames_LocalVariable(t *testing.B) {
	server := &openapi3.Server{
		URL:           "http://{x}.{y}.example.com",
		VariabesInURL: []string{"x", "y"},
	}
	t.ResetTimer()
	for i := 0; i < t.N; i++ {

		p := server.VariabesInURL
		if len(p) != 2 {
			panic("error")
		}
	}
}

@fenollp
Copy link
Collaborator

fenollp commented Dec 2, 2019

Let's keep the code simple & API straightforward: let's just do a private map.
Surely this is not the most expensive operation during your request validations?

@emnnqqs
Copy link
Contributor Author

emnnqqs commented Dec 6, 2019

Yes, this is not the most expensive operation, but this cost can be reduced very easy by introduce a local variable.

@fenollp fenollp mentioned this pull request Apr 25, 2020
7 tasks
@fenollp
Copy link
Collaborator

fenollp commented Mar 19, 2021

If you're still up for it I suggest you tune performance in the legacyrouter itself once #210 is merged.
Some memoization on top of ParameterNames within the router should do it.

@fenollp fenollp closed this Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants