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

perf(primordials): Check the need for SafeArrayIterator each time #80

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

Conversation

petamoriken
Copy link
Contributor

Closes #6

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2023

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

@petamoriken is there a test we could add here?

@petamoriken
Copy link
Contributor Author

@bartlomieju I have no idea what tests to add for primordials.

@bartlomieju
Copy link
Member

Does this PR introduce a change in behavior?

@petamoriken
Copy link
Contributor Author

petamoriken commented Jul 23, 2023

If Array.prototype[Symbol.iterator] and %ArrayIteratorPrototype%.next are not polluted, new SafeArrayIterator(arr) just returns arr without wrapping anything.

I believe this will improve performance, but don't know how to check performance.

@bartlomieju
Copy link
Member

If Array.prototype[Symbol.iterator] and %ArrayIteratorPrototype%.next are not polluted, new SafeArrayIterator(arr) just returns arr without wrapping anything.

I believe this will improve performance, but don't know how to check performance.

You could create a benchmark that uses one object with these props and one without an then see the difference between execution times for say 100_000 items in the iterator.

You can see exmaple benchmarks in core/benches/ops/sync.rs

@petamoriken petamoriken marked this pull request as draft July 24, 2023 01:28
@mmastrac
Copy link
Contributor

@petamoriken Would you still like to land this?

@petamoriken
Copy link
Contributor Author

@mmastrac I am still inclined to work on it myself, but I don't mind having someone else do the work.

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.

I'll try to use the fast path instead of SafeIterator from primordials
4 participants