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

Eliminate ManyVarsDynamicScope as much as possible #5694

Open
headius opened this Issue Apr 12, 2019 · 1 comment

Comments

Projects
None yet
1 participant
@headius
Copy link
Member

headius commented Apr 12, 2019

ManyVarsDynamicScope is our in-place growable DynamicScope, usually used for evals against scopes that need to accommodate additional variables over time. Nearly all other normal scopes will use the right-sized generated DynamicScope subclasses. These are better in almost every case for various reasons:

  • A given scope's code will always be dealing with a specific type of DynamicScope, with all variable accesses inlining right to the access point.
  • There's no array to allocate or indirect through.
  • There are fewer checks needed (array null, array offset, etc) since we always know the exact capacity of the generated scope and the variables are always readable.

We are still using ManyVarsDynamicScope in a number of places where we probably don't need to:

  • For the top-level of a script; a script body like every other non-eval body does not ever change shape, and could be a specific-sized scope.
  • When executing a single "scriptlet" of code, like via evalScriptlet (which makes a new StaticScope every time).
  • When parsing; we create a ManyVarsDynamicScope to give to the parser, but the parser should only need a StaticScope.

For the cases that really do not need a ManyVarsDynamicScope, it seems like an obvious move to go with the right-sized DynamicScope class.

For cases that do need to grow, I would propose that we still can use right-sized scopes, since variables added by subsequent evals again only need to grow the static scope. Then if it turns out we've got a too-small DynamicScope in hand, we request the larger one, copy values over, and proceed. Very few evals introduce new variables over time.

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Apr 12, 2019

cc @enebo... I think most of this will be (hopefully small) tweaks to the parser, so it doesn't need a DynamicScope present when a StaticScope will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.