Improve performance by using v8 serialization #150
Conversation
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Don't push strongly on yourself. ❤️ |
|
Made another change which seems to bring a nice boost Indeed, reusing the same buffer is a significant performance boost. I'm not sure why the linting fails on |
|
For the linting part, I'll talk a look later. |
What means the user must specific the
|
|
Got it, I think I know what it is then, I'll change the readme accordingly. |
Hmm... I don't think only readme change is enough, it's not easy to guess the final output size easily from the developer's side? We'd better hide this detail inside like previous. |
|
Yes, there is a possibility. This happens because We could leverage the What do you think ? |
|
I was able to get a bit more details on the error, I don't understand how/why it's running the code block and outputting the content of the file itself. |
It's virtual file feature of ESLint, and in eslint-mdx, both the whole file content and the code blocks will be linted at the same time. |
Then we could have different strategies for different node versions, what means we can use the previous solution for node <20 for compatibility, but change to use sharedArrayBuffer for node >=20 for better performance. |
|
Hi, I changed the behaviour to only use the SharedArrayBuffer if it can be automatically grown, which does not require any new option to be added |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
===========================================
- Coverage 100.00% 99.52% -0.48%
===========================================
Files 1 1
Lines 207 212 +5
Branches 103 106 +3
===========================================
+ Hits 207 211 +4
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. |
src/index.ts
Outdated
| @@ -501,7 +511,7 @@ function startWorkerThread<R, T extends AnyAsyncFn<R>>( | |||
| : workerPathUrl, | |||
| { | |||
| eval: useEval, | |||
| workerData: { workerPort }, | |||
| workerData: { workerPort, sharedBuffer, useBuffer }, | |||
There was a problem hiding this comment.
| workerData: { workerPort, sharedBuffer, useBuffer }, | |
| workerData: useBuffer ? { sharedBuffer } : { workerPort }, |
There was a problem hiding this comment.
No, we need the buffer anyway for the Atomics.wait
Although we could indeed infer the useBuffer on the other side but I don't know what the relative performance cost is between checking a boolean on an existing object or passing one more property through an object.
I personally prefer the stability of knowing that the properties are here anyway and simplifying the type definition.
There was a problem hiding this comment.
const useBuffer = 'growable' in SharedArrayBuffer.prototypeThe above can be used outside, no need to pass it at all.
We're not using maxByteLength at all, so it should always be growable if this property exists.
|
Thanks @onigoetz for your great job! And sorry for my nitpicking. |
|
And don't worry about the test coverage failing, I'll add some ignore comments accordingly, of course, you can also add them by yourself. |
|
Besides, I want to invite you to join this package. Three options:
Feel free to refuse. |
No worries, it's your project and you'll be the one maintaining what I contribute.
Thanks a lot for the proposition but I have to decline for now. I regularly make small pull requests here and there for some specific things and don't have much time to dedicate to that. If I contribute more regularly on your packages I would be happy to reconsider the offer.
Oh, that's curious. I checked and the branch that is uncovered is the one that is using v8 serialization. Apparently I misunderstood the documentation; a I am experimenting with this option and will update this PR accordingly, my idea at this stage is:
I was able to get this to work but am still stuck on the |
Sure, never mind.
Hah, I was thinking it could be related to Node compatibility, because we could not test on Node 18 and Node 20 at the same time? (I'm still doubting this) |
Oh, I understand something now; I'm really puzzled as to why it does not work yet, I will figure it out :) |
|
I think the PR is ready to be merged at this stage, but I think it should not be merged, because I made a discovery on the benchmark I used; Run 1 Run 2 As you can see, the position of It puzzled me for a while but I discovered the cause when I started looking at memory usage As you can see, it jumps from 149MB to 5432MB when running the benchmark for I switched the benchmarks around and here are the new results: As you can see, the margin of error is very low, for most runs, which seems to indicate that most runs complete within the same timeframe. And the other thing we can see is that this PR is actually slower than the current version of synckit. By looking at the comparison between Fear not! For documentation's sake, this PR does the following:
I also added two tests that ensure we can grow the buffer if relevant or fallback if it does not fit. |
Hi @JounQin
as promised, here is a PR that uses v8 serialization.
I tried using the same benchmark as before and it does indeed improve the performance, but not by as much as I hoped;
As you can see the performance is not as good as what I was able to get on
sync-threadsI did not do any advanced benchmarking yet, but have one or two leads on that:
SharedArrayBufferbetween runs and not initialize a new one each time, I have the impression we could squeeze more performance out of it.sync-threadsandsynckit(one more object wrapping the message) maybe this has an impact?I can make a few more tests but I'll have to do that a bit later