Skip to content

refactor: remove return value from v8 throw - #1355

Open
ValoChet wants to merge 1 commit into
uNetworking:masterfrom
ValoChet:refactorThrow
Open

ValoChet wants to merge 1 commit into
uNetworking:masterfrom
ValoChet:refactorThrow

Conversation

@ValoChet

@ValoChet ValoChet commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

When throwing a v8 exception, there is no need to set the return value as v8 ignores it.

This PR removes args.GetReturnValue().Set() from throwing paths.
And uses NewFromUtf8Literal() instead of NewFromUtf8().ToLocalChecked() for static error messages.

It also updates Callback and NativeString to throw directly inside the constructor instead of in isInvalid().
And removes the args argument from isInvalid(), making it a simple getter.

This PR only makes the code cleaner, with no performance changes.

@github-actions

Copy link
Copy Markdown

Benchmark: 9fbe959 against 6aa8a01

Scenario Base req/s Head req/s Head / base Rounds Noise Busy CPU per request
http/hello-world 116408 117460 1.008x (+0.8%) 1.01 to 1.01 ±2.7% 100% / 100% 8.58 / 8.51 us (0.992x)
http/headers 104064 103860 0.998x (-0.2%) 0.99 to 1.01 ±2.5% 100% / 100% 9.60 / 9.62 us (1.002x)
http/json-post 🏆 86440 88335 1.024x (+2.4%) 1.01 to 1.03 ±1.7% 100% / 100% 11.55 / 11.31 us (0.977x)
http/cached 128637 128864 1.002x (+0.2%) 1.00 to 1.01 ±0.7% 100% / 100% 7.77 / 7.75 us (0.998x)
ws/echo-20b 118858 118227 0.996x (-0.4%) 0.99 to 1.00 ±1.8% 100% / 100% 8.41 / 8.45 us (1.004x)
ws/echo-4kb 104449 104625 1.002x (+0.2%) 1.00 to 1.01 ±1.0% 100% / 100% 9.56 / 9.55 us (0.998x)

4 rounds of 3s per scenario, each round loading base, head and a second process of each one, from a copy of its build, after the other, in an order that changes every round. "Head / base" is the median of the per-round ratios of req/s and "Rounds" their range. "Noise" is how far base/base and head/head, the same code on both sides, got from 1 in this same run: that is what the machine did, so a row is marked only when the median moved further than that, at least 2%, and every round moved the same way: 👀 slower, 🏆 faster. "Busy" is the cpu time of the server process over the round and "CPU per request" that time divided by the requests it answered, with the head/base ratio judged against its own same-code band and bold when it cleared it. When busy is well under 100% the load generator set the pace, the req/s are its and the cpu per request is the column to read. Only the ratios are comparable across runs, the absolute figures depend on the runner.

Node v26.12.0, AMD EPYC 7763 64-Core Processor, 4 cores (server on cpu 0, load on 2,3), http_load_test and load_test with 100 connections.

@ValoChet

Copy link
Copy Markdown
Contributor Author

I see some other areas we could refactor to clean up the codebase. Let me know if you're open to that.

@uNetworkingAB

Copy link
Copy Markdown
Contributor

It doesn't simplify anything. What simplifies is that you have (apart from your description) also moved the place of exception throwing from within isInvalid, to instead inside the constructor. That's not due to skipping args.GetReturnValue().Set(, that's due to you changing the interface.

Your PR should really read: "I changed the interface of all Callbacks and NativeStrings to throw inside the constructor instead of in the getter isInvalid".

None of that is related to V8 ThrowException. I don't like this disconnect between what you say you do, and what you actually do. Your PR should be a simple 1-to-1 removal of the args.GetReturnValue().Set( if your PR description is like that. But you also changed a bunch of other things like NewFromUtf8Literal. I'm not saying this broke anything, but you don't really describe your PRs thruthfully.

Also, I don't like it because it makes no difference. Not setting a return value in the throw path has negligible/irrelevant perf. impact since it already is the very slow path. And It's charming to have legacy V8 usage where that is still backwards compatible.

@ValoChet

Copy link
Copy Markdown
Contributor Author

You are right, my PR message was not very descriptive of the actual changes.
I am not very good at writing descriptions, I have updated it.

This PR does not give any performance gain (it may help the compiler).
It only simplify/modernise the code base, but it is still important.
Feel free to close it if you prefer to keep the current implementation.

@uNetworkingAB

Copy link
Copy Markdown
Contributor

but it is still important.

It's not.

This branch has not been deployed

No deployments
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