Skip to content

Conversation

@tsctx
Copy link
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
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
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
Member Author

tsctx commented May 21, 2024

Why not fixed_queue from node core?

There is no particular reason for this.

@tsctx
Copy link
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
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
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
Member Author

tsctx commented May 21, 2024

do you have benchmarks?

What about?

@tsctx
Copy link
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
Member

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

@tsctx
Copy link
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
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
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
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
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
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.

4 participants