Skip to content

Address Gemini code review comments on PR #494 #496

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 6, 2025
Merged

Address Gemini code review comments on PR #494 #496

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ccddec
Address Gemini code review comments on PR #494
Oct 6, 2025
afd2096
Add explanation for not using beforeAll in benchmarks
Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"test:autobahn": "cd test/autobahn && ./run-wstest.js",
"bench": "vitest bench --run --config vitest.bench.config.mjs",
"bench:baseline": "node test/benchmark/track-performance.mjs save",
"bench:compare": "node test/benchmark/track-performance.mjs compare",
"bench:check": "node test/benchmark/track-performance.mjs check",
"lint": "eslint lib/**/*.js test/**/*.js",
"lint:fix": "eslint lib/**/*.js test/**/*.js --fix"
Expand Down
30 changes: 14 additions & 16 deletions test/benchmark/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
{
"timestamp": "2025-10-06T16:43:17.731Z",
"timestamp": "2025-10-06T17:27:59.641Z",
"results": {
"WebSocketConnection Performance 3866ms": {
"create connection instance": 28323.39,
"send small UTF-8 message": 28201.35,
"send medium UTF-8 message (1KB)": 24615.97,
"send binary message (1KB)": 24889.31,
"send ping frame": 31562.37,
"send pong frame": 32393.72
"WebSocketConnection Performance 7359ms": {
"create connection instance": 28847.63,
"send small UTF-8 message": 914160.4,
"send medium UTF-8 message (1KB)": 108086.21,
"send binary message (1KB)": 222918.81,
"send ping frame": 2375454.67,
"send pong frame": 1935975.19
},
"WebSocketFrame Performance 10001ms": {
"serialize small text frame (17 bytes, unmasked)": 4427042.23,
"serialize small text frame (17 bytes, masked)": 3005215.87,
"serialize medium binary frame (1KB)": 4239270.36,
"serialize large binary frame (64KB)": 4024552.34
},
"WebSocketConnection Performance": {},
"WebSocketFrame Performance": {}
"WebSocketFrame Performance 9745ms": {
"serialize small text frame (17 bytes, unmasked)": 4240401.67,
"serialize small text frame (17 bytes, masked)": 3070532.44,
"serialize medium binary frame (1KB)": 4418217.61,
"serialize large binary frame (64KB)": 3918678.38
}
}
}
43 changes: 16 additions & 27 deletions test/benchmark/connection-operations.bench.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,51 +3,40 @@ import WebSocketConnection from '../../lib/WebSocketConnection.js';
import { MockSocket } from '../helpers/mocks.mjs';

describe('WebSocketConnection Performance', () => {
// Pre-allocate messages and buffers outside benchmarks
const smallMessage = 'Hello, WebSocket!';
const mediumMessage = 'x'.repeat(1024);
const binaryBuffer = Buffer.alloc(1024);

// Pre-create connection for send operations (created once, reused across all iterations)
const sharedSocket = new MockSocket();
const sharedConnection = new WebSocketConnection(sharedSocket, [], 'echo-protocol', false, {});
sharedConnection._addSocketEventListeners();
sharedConnection.state = 'open';
Copy link

@gemini-code-assist gemini-code-assist bot Oct 6, 2025

Choose a reason for hiding this comment

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

medium

While the current setup works, using a beforeAll hook is more idiomatic for setting up state that is shared across all benchmarks in a suite. This makes the setup logic more explicit and aligns with common testing practices in frameworks like Vitest.

You could refactor this part like so:

import { bench, describe, beforeAll } from 'vitest';
// ...

describe('WebSocketConnection Performance', () => {
  // ... (message and buffer allocations)

  let sharedConnection;

  beforeAll(() => {
    // Pre-create connection for send operations (created once, reused across all iterations)
    const sharedSocket = new MockSocket();
    sharedConnection = new WebSocketConnection(sharedSocket, [], 'echo-protocol', false, {});
    sharedConnection._addSocketEventListeners();
    sharedConnection.state = 'open';
  });

  // ... (benchmarks using sharedConnection)
});

This change would require importing beforeAll from vitest.


bench('create connection instance', () => {
const socket = new MockSocket();
const connection = new WebSocketConnection(socket, [], 'echo-protocol', false, {});
connection._addSocketEventListeners();
});

bench('send small UTF-8 message', () => {
const socket = new MockSocket();
const connection = new WebSocketConnection(socket, [], 'echo-protocol', false, {});
connection._addSocketEventListeners();
connection.state = 'open';
connection.sendUTF('Hello, WebSocket!');
sharedConnection.sendUTF(smallMessage);
});

bench('send medium UTF-8 message (1KB)', () => {
const socket = new MockSocket();
const connection = new WebSocketConnection(socket, [], 'echo-protocol', false, {});
connection._addSocketEventListeners();
connection.state = 'open';
const message = 'x'.repeat(1024);
connection.sendUTF(message);
sharedConnection.sendUTF(mediumMessage);
});

bench('send binary message (1KB)', () => {
const socket = new MockSocket();
const connection = new WebSocketConnection(socket, [], 'echo-protocol', false, {});
connection._addSocketEventListeners();
connection.state = 'open';
const buffer = Buffer.alloc(1024);
connection.sendBytes(buffer);
sharedConnection.sendBytes(binaryBuffer);
});

bench('send ping frame', () => {
const socket = new MockSocket();
const connection = new WebSocketConnection(socket, [], 'echo-protocol', false, {});
connection._addSocketEventListeners();
connection.state = 'open';
connection.ping();
sharedConnection.ping();
});

bench('send pong frame', () => {
const socket = new MockSocket();
const connection = new WebSocketConnection(socket, [], 'echo-protocol', false, {});
connection._addSocketEventListeners();
connection.state = 'open';
connection.pong();
sharedConnection.pong();
});
});
6 changes: 5 additions & 1 deletion test/benchmark/track-performance.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,17 @@ function parseBenchmarkOutput(output) {
// Detect suite name
if (line.includes('> WebSocket')) {
currentSuite = line.match(/> (.*)/)[1].trim();
results[currentSuite] = {};
// Don't initialize suite here - wait until first benchmark is found
}

// Parse benchmark results
const benchMatch = line.match(/^\s*[·•]\s+(.+?)\s+(\d+(?:,\d+)*(?:\.\d+)?)\s/);
if (benchMatch && currentSuite) {
const [, name, hz] = benchMatch;
// Lazily initialize suite only when first benchmark is found
if (!results[currentSuite]) {
results[currentSuite] = {};
}
results[currentSuite][name.trim()] = parseFloat(hz.replace(/,/g, ''));
}
}
Expand Down
1 change: 0 additions & 1 deletion vitest.bench.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { defineConfig } from 'vitest/config';

export default defineConfig({
test: {
include: ['test/benchmark/**/*.bench.mjs'],
benchmark: {
include: ['test/benchmark/**/*.bench.mjs'],
exclude: ['node_modules/', 'test/unit/', 'test/integration/'],
Expand Down