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

Replace JsValue[] arguments with Arguments struct #1135

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Apr 18, 2022

Just pushing this into queue so it won't get lost. Concept is based on how YantraJS handles arguments.

Idea is to replace JsValue[] arguments in used method signatures for call receiver and instead use a custom struct that hides what's the backing way of handling things. Arguments gives a better abstraction and offers the same indexing and At() capabilties.

For four or less parameters they are just fields and don't require array handling, this way JsValueArrayPool pretty much becomes obsolete. For larger parameter sets (which should be unusual, 4 is largest handled by EcmaScript built-in functions).

The code gets simpler, no need for array pooling and performance doesn't change that much - I think it's in margin of error. API is now also cleaner and passing parameters always goes via Arguments() constructor and no need to think about array pooling and releasing of items.

Things to think, should we also cover thisObject in Arguments and make all calls take just Call(in Arguments arguments). This would also allow passing more date in the future if needed. Many methods require engine reference which is a reason from them not be static.

BREAKING CHANGE.

@scarletquasar
Copy link

Why not implement an overload instead of replacing? You can deprecate with time

@lahma
Copy link
Collaborator Author

lahma commented Sep 27, 2022

This might not be that good idea after all. This will probably make stack grow more and we've been recently battling with making stack small enough to allow deep nesting and recursions.

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