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

Optimise AST rewriting #7583

Merged
merged 1 commit into from
Mar 2, 2021
Merged

Conversation

systay
Copy link
Collaborator

@systay systay commented Mar 2, 2021

When visiting slice elements, we were creating a closure over the index variable, like so:

case *IndexHints:
	for x, el := range n.Indexes {
		a.apply(node, el, func(newNode, container SQLNode) {
			container.(*IndexHints).Indexes[x] = newNode.(ColIdent) // x is closed over
		})
	}

this PR changes these code snippets to be explicit about the parameter, like this:

case *IndexHints:
	for x, el := range n.Indexes {
		a.apply(node, el, func(idx int) func(SQLNode, SQLNode) {
			return func(newNode, container SQLNode) {
				container.(*IndexHints).Indexes[idx] = newNode.(ColIdent)
			}
		}(x))

Even though the latter looks a little worse, what with the creation of a function for each element, the benchmarks show a small but significant improvement:

name                     old time/op  new time/op  delta
Normalize-16             4.64µs ± 1%  4.52µs ± 1%  -2.59%  (p=0.000 n=9+10)
VisitLargeExpression-16  15.7µs ± 1%  14.7µs ± 3%  -6.14%  (p=0.000 n=10+10)

Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay systay removed the request for review from sougou March 2, 2021 14:11
@harshit-gangal harshit-gangal merged commit d2f939e into vitessio:master Mar 2, 2021
@harshit-gangal harshit-gangal deleted the deep-clone branch March 2, 2021 16:27
@askdba askdba added this to the v10.0 milestone Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants