Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

EXPERIMENTAL: feat(ops): String fast ops, fast op_base64_decode #16014

Closed

Conversation

aapoalas
Copy link
Collaborator

@aapoalas aapoalas commented Sep 24, 2022

This PR introduces String support to fast ops, and trials a method of making ZeroCopyBuf-returning ops fast-compliant by having them return a ResourceId and (through an out-buffer) a buffer size. JS side will then create a buffer of that size and call a finisher op that writes the Vec to the passed-in buffer and drops the Vec.

The fast-compliant ZeroCopyBuf returning functions are a pretty easy idea and shouldn't really cause any issues. The experimental nature of this PR is in the String support for fast ops.

First of all, the support required adding a v8::Isolate pointer to the OpState which is already really dirty. Additionally, while @littledivy has verified that it is possible to get the string content, it may not work in all cases. @AaronO raised the possibility that getting the string content for string ropes might cause V8 to do string combining, meaning that suddenly the fast op would be proccing heap allocations and that might cause V8 to panic.

Locally tests pass so nothing is breaking directly, but tests likely never use roped strings so this is no guarantee. Additionally, op_base64_decode is apparently not used at all, anywhere.

@aapoalas aapoalas force-pushed the feat/ops-string-fast-ops-base64-decode branch 4 times, most recently from c14d70d to 8683c87 Compare October 2, 2022 16:14
@aapoalas aapoalas force-pushed the feat/ops-string-fast-ops-base64-decode branch from 7ef6773 to 7793d7d Compare October 9, 2022 21:46
@aapoalas aapoalas force-pushed the feat/ops-string-fast-ops-base64-decode branch from 7793d7d to 1ea75c4 Compare October 9, 2022 21:49
@bartlomieju
Copy link
Member

@littledivy can we land this PR?

@aapoalas
Copy link
Collaborator Author

No

@aapoalas
Copy link
Collaborator Author

aapoalas commented Nov 8, 2022

I'll close this for now, at least.

The branch did show fast calls using Strings, but apparently not during benchmarking. I could not see any improvements in performance.

The most important contribution in this branch is probably the work to eliminate a DoubleBorrow error from ops/lib.rs, but I've already mostly forgotten which change was actually the relevant part fixing the issue.

@aapoalas aapoalas closed this Nov 8, 2022
@aapoalas aapoalas deleted the feat/ops-string-fast-ops-base64-decode branch December 22, 2022 18:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants