Skip to content

websocket: use FixedQueue instead of Set#3283

Merged
tsctx merged 8 commits intonodejs:mainfrom
tsctx:websocket/use-linked-list
May 22, 2024
Merged

websocket: use FixedQueue instead of Set#3283
tsctx merged 8 commits intonodejs:mainfrom
tsctx:websocket/use-linked-list

Conversation

@tsctx
Copy link
Copy Markdown
Member

@tsctx tsctx commented May 21, 2024

@tsctx tsctx changed the title websocket: use linkedlist insteadof Set websocket: use linkedlist instead of Set May 21, 2024
@github-actions

This comment was marked as off-topic.

@tsctx tsctx force-pushed the websocket/use-linked-list branch from 4dfe172 to cc4f3de Compare May 21, 2024 09:02
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Copy Markdown
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not fixed_queue from node core?

@tsctx
Copy link
Copy Markdown
Member Author

tsctx commented May 21, 2024

Why not fixed_queue from node core?

There is no particular reason for this.

@tsctx
Copy link
Copy Markdown
Member Author

tsctx commented May 21, 2024

In this use case, I prefer the simple linkedlist; Initialization of the array of 2048 entries takes time :)

@ronag
Copy link
Copy Markdown
Member

ronag commented May 21, 2024

In this use case, I prefer the simple linkedlist; Initialization of the array of 2048 entries takes time :)

I think that is amortized quite quickly...

Copy link
Copy Markdown
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have benchmarks?

@tsctx
Copy link
Copy Markdown
Member Author

tsctx commented May 21, 2024

do you have benchmarks?

What about?

@tsctx
Copy link
Copy Markdown
Member Author

tsctx commented May 21, 2024

If it was about sending data, it would be 1.2 times faster since we changed to zero-copy.

@KhafraDev
Copy link
Copy Markdown
Member

Yeah, sending messages. Do you have the data to backup the 1.2x faster claim?

@tsctx
Copy link
Copy Markdown
Member Author

tsctx commented May 21, 2024

okey, look this

https://github.com/nodejs/undici/blob/370f8cbf20ccf80dc3dbc0a3c6d39a041b1eb631/benchmarks/websocket/websocket-send-buffer.mjs

benchmark      time (avg)             (min … max)       p75       p99      p999
------------------------------------------------- -----------------------------
• send
------------------------------------------------- -----------------------------
undici      1'658 µs/iter   (1'305 µs … 4'145 µs)  1'673 µs  3'043 µs  4'145 µs <- before
undici      1'467 µs/iter   (1'187 µs … 6'165 µs)  1'442 µs  4'022 µs  6'165 µs <- after

@tsctx
Copy link
Copy Markdown
Member Author

tsctx commented May 21, 2024

In this use case, I prefer the simple linkedlist; Initialization of the array of 2048 entries takes time :)

I think that is amortized quite quickly...

Rewrite with fixed_queue!

Copy link
Copy Markdown
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is queue being lazily created?

Copy link
Copy Markdown
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are failing

@tsctx
Copy link
Copy Markdown
Member Author

tsctx commented May 21, 2024

I don't have time right now, so I will rewrite it in another PR.

@tsctx tsctx force-pushed the websocket/use-linked-list branch from 8667b10 to c05988c Compare May 21, 2024 21:13
@tsctx tsctx force-pushed the websocket/use-linked-list branch from 468d2d0 to a88052b Compare May 21, 2024 21:34
Comment thread lib/web/websocket/sender.js Outdated
Comment thread lib/web/websocket/sender.js Outdated
Comment thread lib/web/websocket/sender.js Outdated
Comment thread lib/web/websocket/sender.js Outdated
Comment thread lib/web/websocket/sender.js Outdated
kodiakhq Bot referenced this pull request in X-oss-byte/Nextjs Jun 18, 2024