memory optimization #90
Conversation
websocket_server/app.js
Outdated
| app.get('/system-overview', (req, res) => { | ||
| res.json(getSystemOverview(rooms)) | ||
| }) |
There was a problem hiding this comment.
Can we expose those also as prometheus metrics in the /metrics endpoint instead? If there is something we want to expose separately it should also be limited to requests where the METRICS_TOKEN is passed
juliusknorr
force-pushed
the
feat/memory-optimization
branch
from
7c71b9b to
8a4a86b
Compare
websocket_server/roomData.js
Outdated
| export const roomUsers = new Map() | ||
| export const lastEditedUser = new Map() | ||
| const INACTIVE_THRESHOLD = 30 * 60 * 1000 // 30 minutes | ||
| const MAX_ROOMS = 100 |
There was a problem hiding this comment.
I'm not to sure we should have an artificial limit in here and the tokens, might become hard to debug if that causes problems. Or can we be sure that once an entry is disposed existing sessions will properly continue to work and just get back into the cache?
There was a problem hiding this comment.
Yes, it's one of the options to ensure safe bounded storage like written in the docs https://www.npmjs.com/package/lru-cache
Some of the benefits:
- Performance optimization:: Allocate all the required memory up-front
- LRU behavior: The cache will store a maximum of 100 rooms. When the 101 room is added, the least recently used item will be removed from the cache
I think better to set to control the memory but the value can be discussed and it will not affect the function because we will always have the data get back from file or re-decode the token
There was a problem hiding this comment.
Good points about when the roomData suddenly got wiped out, what will happen with all users in that room, let me think about that @juliushaertl
hweihwang
force-pushed
the
feat/memory-optimization
branch
from
8a4a86b to
6f1a310
Compare
websocket_server/roomData.js
Outdated
| super() | ||
| this.apiService = apiService | ||
| this.client = createClient({ | ||
| url: process.env.REDIS_URL || 'redis://localhost:6379', |
There was a problem hiding this comment.
Please document in .env.example
websocket_server/server.js
Outdated
| @@ -22,53 +24,66 @@ const { | |||
| TLS, | |||
| TLS_KEY: keyPath, | |||
| TLS_CERT: certPath, | |||
| STORAGE_STRATEGY = 'redis', | |||
There was a problem hiding this comment.
Also to document in .env.example
There was a problem hiding this comment.
We should probably default to LRU
| STORAGE_STRATEGY = 'redis', | |
| STORAGE_STRATEGY = 'lru', |
|
And the node build needs some fixes, strangely npm ci works fine locally for me |
|
Thank @juliushaertl, about to wrap up and push another including the scaling socket server on multiple nodes part using Redis Stream Adapter |
hweihwang
force-pushed
the
feat/memory-optimization
branch
from
6f1a310 to
ccc4ab1
Compare
websocket_server/apiService.js
Outdated
There was a problem hiding this comment.
I think this file's a duplicate!? we also have ApiService.js
There was a problem hiding this comment.
Thank you @grnd-alt , not sure why running well at my local, trying to fix
|
@hweihwang Can you squash your commits into reasonable chunks (at least to get the commits with the same message merged into one atomic change) |
hweihwang
force-pushed
the
feat/memory-optimization
branch
2 times, most recently
from
dce7c15 to
77f20fd
Compare
|
Sorry another conflict in the package-lock to resolve 🙈 |
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
hweihwang
force-pushed
the
feat/memory-optimization
branch
from
77f20fd to
93dac12
Compare
No description provided.