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

internal/compile: build index of local vars #576

Merged
merged 1 commit into from
Feb 5, 2025
Merged

Conversation

adonovan
Copy link
Collaborator

@adonovan adonovan commented Feb 5, 2025

This eliminates the linear scan over locals in the interpreter when processing named arguments. This reduces the new benchmark from 1081ns to 843ns (-22%).

(This was inspired by recent work in the Java-based implementation, but findParams seemed to be a bigger hit in that case, possibly because of the two extra levels of indirection involved in the length comparison (hot path) of a Java array of Java strings, versus Go where they are all in the same cache line.)

This eliminates the linear scan over locals in the interpreter
when processing named arguments. This reduces the new benchmark
from 1081ns to 843ns (-22%).

(This was inspired by recent work in the Java-based implementation,
but findParams seemed to be a bigger hit in that case, possibly
because of the two extra levels of indirection involved in the
length comparison (hot path) of a Java array of Java strings,
versus Go where they are all in the same cache line.)
Copy link
Collaborator

@tetromino tetromino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the idea (just yesterday I thought of adding the same map to the java starlark interpreter in MethodDescriptor :) but I'm probably not qualified to judge the go implementation

@adonovan adonovan merged commit 492d367 into master Feb 5, 2025
27 checks passed
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.

3 participants