improve page load performance of large amount urls #5025

vishalsabhaya · 2024-08-17T06:49:42Z

@CommanderStorm @louislam

fix loop to async
query n+1 problem

Tick the checkbox if you understand [x]:

I have read and understand the pull request rules.

Description

Resolves #5129

testing results:
monitor url count: 415
current:
cpu: 1 memory 2 gb: 1 min 44 sec

after changes

database: aws rds marinadb db.t3.micro(memory: 1gb, cpu: 2vcpu)

with docker run on mac:
cpu: 0.5 memory 1 gb: 21 sec
cpu: 1 memory 2 gb: 11 sec
cpu: 1.5 memory 3 gb: 10 sec
cpu: 2 memory 4 gb: 10 sec

with ESC Farget(Linux/ARM64):
cpu: 0.5 memory 1 gb: 31 sec
cpu: 1 memory 2 gb: 6 sec
cpu: 2 memory 4 gb: 5 sec
Fixes #3494

Type of change

Performance improvement

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and tested it
I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
My changes generates no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

1) fix loop to async 2) query n+1 problem

moved changes while testing

vishalsabhaya · 2024-08-19T02:44:03Z

@CommanderStorm @louislam

I debug & fix performance issues for below
3) insert/update/delete/pause/resume monitor.

monitor url count: 415
database: aws rds marinadb db.t3.micro(memory: 1gb, cpu: 2vcpu)
current:
page load time + 500ms to 1 sec
it always fetches the latest data from the database & builds json. i felt that it was extra time.

after changes:
< 700ms

I just updated the target record with the front end instead of all records.

CommanderStorm

Sorry for the late review, I am currently quite stressed work-wise..

While the ~16x(!) performance improvement in this PR is great, I don't quite like how you implemented some parts.
(most parts are fine)

preloadData is quite clunky
=> please move this into a single object per toJSON that we pass to the function instead of the current way of passing a lookup-object into the function.
The current way is quite likely error prown and without tests whatsoever way in this part extremely risky.

Only passing an object also has the advantage of allowing better type-checking via adding what is passed in to the doc-comment.

I have also left some comments below

CommanderStorm · 2024-08-21T21:35:45Z

server/model/monitor.js

+        );
+
+        // Organize preloaded data for efficient access
+        return {


the .reduce calls with object-changes here are really hard to parse for me and thus likely other devs.
If there were a bug in here, I would not be able to tell.

Please move this into regular for-loops.
While moving this, please also check if Map is not more appropriate.
I found this blog post with some claims that this might increase performance (if that code happends to be a factor we are spending a part of the remaining 700ms..)

@CommanderStorm
understand let me double check with it.

@CommanderStorm
I simplified logic. can you review one more time.

CommanderStorm · 2024-08-21T21:46:34Z

server/uptime-kuma-server.js

-        let list = await this.getMonitorJSONList(socket.userID);
-        this.io.to(socket.userID).emit("monitorList", list);
-        return list;
+    async sendMonitorList(socket, op = OPERATIONS.LIST, monitorID = null) {


This seems pretty strange
the whole part why you added op here and above seems to be to create a fast-path for OPERATIONS.DELETE, right?

Instead of doing this, please just remove the calls for OPERATIONS.DELETE if we really don't use the list there..
Cleans up the code a lot..

@CommanderStorm
its used as common method. based on this parameter we will identified operation & update same to frontend list. you can refer socket.js changes

while page load it will use OPERATIONS.LIST as default.
while insert or update. we will add only 1 URL object to list.
while delete no need to send list because we have to delete from fronted list. from backend already deleted.

server/uptime-kuma-server.js

server/model/monitor.js

server/server.js

src/mixins/socket.js

vishalsabhaya · 2024-08-22T00:53:31Z

@CommanderStorm

Thank you for your effert to review my pr. i am really appreciate your hard work for community.
I can't understand below comment can you elaborate?

While the ~16x(!) performance improvement in this PR is great, I don't quite like how you implemented some parts.
(most parts are fine)

preloadData is quite clunky
=> please move this into a single object per toJSON that we pass to the function instead of the current way of passing a lookup-object into the function.
The current way is quite likely error prown and without tests whatsoever way in this part extremely risky.

Only passing an object also has the advantage of allowing better type-checking via adding what is passed in to the doc-comment.

vishalsabhaya · 2024-08-22T04:20:43Z

@CommanderStorm @louislam

I found 1 more thing. it will improve more seconds.

marked realtime dot is maximum 43. why we are fetching 100 records for each monitor? is there any reason?
currently I am testing with 834 monitor
limit 100 takes: 13 sec
limit 43 takes: 7 sec

CommanderStorm · 2024-08-23T12:31:59Z

is there any reason?

I can imagine two:

What happens if you zoom out?
What happens on high-dpi devices?

If we change that part, lets please do this in a second PR ^^

vishalsabhaya · 2024-08-26T01:22:55Z

is there any reason?

I can imagine two:

What happens if you zoom out?

What happens on high-dpi devices?

If we change that part, lets please do this in a second PR ^^

yes, you are correct. i didn't think about it. we will do in next pr. but we should restrict max span. because for detail information already provided graph.

move inner promise to outer promise

made code clean make it faster

move path to preload function

vishalsabhaya · 2024-09-04T11:47:15Z

@CommanderStorm
i seen something strange. did you notice ever?

i did testing with 800 records.

when I remove below line. all monitoring is working fine but current case most of monitor is stopped. monitoring data is not updating so monitor is not starting.
await sleep(getRandomInt(300, 1000));

startMonitors does not have await that's why?

CommanderStorm · 2024-09-04T12:27:05Z

Sorry, above comment does not quite parse for me.

As far as I understand that line, this just means that the monitors are started one after another with random intervals in between.

The way this is executed, starting monitors should happen after the server started listening, but must not have completely/partially completed before starting the versionCheck.
That this is not done in a Promise.all makes a thundering heard problem less likely. The speed of starting them is constant though.

As far as I understand async js (feel free to correct), that sounds partially reasonable.
Changing the monitor startup to be smarter can be done, but let's separate this Into its own PR. ^^

revert to original insted of map

vishalsabhaya · 2024-09-18T07:37:49Z

@CommanderStorm
now all changes are done. can you review & guide me further? we should merge after review?

vishalsabhaya · 2024-09-30T02:19:48Z

@CommanderStorm
is there any new changes?
sorry to ping you again. i know you are very busy

CommanderStorm · 2024-09-30T02:24:22Z

Sorry, got back from vacation on monday and am still picking up the pieces work wise and had not time for such a large PR.
I can answer issues or merge small PRs on the train but for this one, I need an hour of free time to look if everything works as intended.

vishalsabhaya · 2024-09-30T02:26:24Z

@CommanderStorm
ahhh, understood. take your time.

CommanderStorm

I have left a few nits to apply, but mostly LGTM 🎉

Note

This PR is part of the v2.0 merge window => see #4500 for the bugs that need to be addressed before we can ship this release ^^

_{All changes in this PR are small and uncontroversial ⇒ merging with junior maintainer approval}

server/model/monitor.js

src/mixins/socket.js

server/uptime-kuma-server.js

louislam · 2024-10-12T16:26:32Z

server/server.js

    for (let monitorID in monitorList) {
-        await sendHeartbeatList(socket, monitorID);
+        monitorPromises.push(sendHeartbeatList(socket, monitorID));
+        monitorPromises.push(Monitor.sendStats(io, monitorID, user.id));


@vishalsabhaya Just checking this pr. Is this code correct? Because the function returns nothing.

improve page load performance of large amount urls

84f261c

1) fix loop to async 2) query n+1 problem

vishalsabhaya mentioned this pull request Aug 17, 2024

On Page reload monitors do not load #3494

Closed

2 tasks

vishalsabhaya added 2 commits August 18, 2024 18:06

missed changes from other branch

32f2fe1

moved changes while testing

performance improve while insert/update/delete

ae7f01b

CommanderStorm requested changes Aug 21, 2024

View reviewed changes

CommanderStorm added area:api concearning the api or automation pr:please address review comments this PR needs a bit more work to be mergable labels Aug 21, 2024

CommanderStorm mentioned this pull request Aug 24, 2024

feat: implement very crude and bare-bones RSS feed #5047

Merged

7 tasks

vishalsabhaya added 5 commits September 3, 2024 16:40

removed unneccesory comment

bb499e2

depricate inner promise

96a1010

move inner promise to outer promise

object to map to make it faster

a81317f

made code clean make it faster

update fix linting error

b0281e4

remove async from tojson method

043f1a2

move path to preload function

vishalsabhaya added 2 commits September 5, 2024 17:16

optimisation for creation, update, list and delete

cb1b414

fix linting error

59aea39

github-actions bot added the needs:resolve-merge-conflict label Sep 5, 2024

Merge branch 'master' into load-performance-improve

de48fe7

github-actions bot removed the needs:resolve-merge-conflict label Sep 18, 2024

linting error fix

39f3e63

vishalsabhaya mentioned this pull request Sep 18, 2024

Monitor Conditions #5048

Merged

7 tasks

notificationIDList not display properly

416ba88

revert to original insted of map

vishalsabhaya requested a review from CommanderStorm September 19, 2024 01:49

CommanderStorm mentioned this pull request Sep 26, 2024

URLs not showing in uptime dashboard #5129

Closed

1 task

CommanderStorm mentioned this pull request Oct 4, 2024

Web UI Hanging #3276

Open

2 tasks

CommanderStorm added pr:needs review this PR needs a review by maintainers or other community members and removed pr:please address review comments this PR needs a bit more work to be mergable labels Oct 4, 2024

CommanderStorm added 3 commits October 6, 2024 02:50

inlined two functions which don't not need to exist

4b3cfb6

Fixed a typo I introduced in the last commit

5334823

Removed unnessesary returns

f1577a4

CommanderStorm approved these changes Oct 6, 2024

View reviewed changes

CommanderStorm added 3 commits October 6, 2024 03:21

Apply suggestions from code review

ede8282

Merge branch 'master' into load-performance-improve

90dd60d

Fixed a typo

d321b77

CommanderStorm added this to the 2.0.0 milestone Oct 6, 2024

CommanderStorm merged commit d0067a0 into louislam:master Oct 6, 2024
18 checks passed

CommanderStorm mentioned this pull request Oct 6, 2024

v2.0.0-Release <- Read this for performance problems #4500

Closed

16 tasks

louislam reviewed Oct 12, 2024

View reviewed changes

louislam added the question Further information is requested label Oct 12, 2024

julian-piehl mentioned this pull request Oct 13, 2024

fix: showing the notification activation switch properly #5196

Merged

7 tasks

louislam mentioned this pull request Oct 29, 2024

v2 tag values missing in UI #5277

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve page load performance of large amount urls #5025

improve page load performance of large amount urls #5025

vishalsabhaya commented Aug 17, 2024 •

edited by CommanderStorm

Loading

vishalsabhaya commented Aug 19, 2024 •

edited

Loading

CommanderStorm left a comment •

edited

Loading

CommanderStorm Aug 21, 2024

vishalsabhaya Aug 22, 2024

vishalsabhaya Sep 4, 2024

CommanderStorm Aug 21, 2024

vishalsabhaya Aug 22, 2024

vishalsabhaya commented Aug 22, 2024

vishalsabhaya commented Aug 22, 2024

CommanderStorm commented Aug 23, 2024

vishalsabhaya commented Aug 26, 2024

vishalsabhaya commented Sep 4, 2024

CommanderStorm commented Sep 4, 2024

vishalsabhaya commented Sep 18, 2024

vishalsabhaya commented Sep 30, 2024

CommanderStorm commented Sep 30, 2024 •

edited

Loading

vishalsabhaya commented Sep 30, 2024

CommanderStorm left a comment

louislam Oct 12, 2024 •

edited

Loading

improve page load performance of large amount urls #5025

improve page load performance of large amount urls #5025

Conversation

vishalsabhaya commented Aug 17, 2024 • edited by CommanderStorm Loading

Description

Type of change

Checklist

Screenshots (if any)

vishalsabhaya commented Aug 19, 2024 • edited Loading

CommanderStorm left a comment • edited Loading

Choose a reason for hiding this comment

CommanderStorm Aug 21, 2024

Choose a reason for hiding this comment

vishalsabhaya Aug 22, 2024

Choose a reason for hiding this comment

vishalsabhaya Sep 4, 2024

Choose a reason for hiding this comment

CommanderStorm Aug 21, 2024

Choose a reason for hiding this comment

vishalsabhaya Aug 22, 2024

Choose a reason for hiding this comment

vishalsabhaya commented Aug 22, 2024

vishalsabhaya commented Aug 22, 2024

CommanderStorm commented Aug 23, 2024

vishalsabhaya commented Aug 26, 2024

vishalsabhaya commented Sep 4, 2024

CommanderStorm commented Sep 4, 2024

vishalsabhaya commented Sep 18, 2024

vishalsabhaya commented Sep 30, 2024

CommanderStorm commented Sep 30, 2024 • edited Loading

vishalsabhaya commented Sep 30, 2024

CommanderStorm left a comment

Choose a reason for hiding this comment

louislam Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

vishalsabhaya commented Aug 17, 2024 •

edited by CommanderStorm

Loading

vishalsabhaya commented Aug 19, 2024 •

edited

Loading

CommanderStorm left a comment •

edited

Loading

CommanderStorm commented Sep 30, 2024 •

edited

Loading

louislam Oct 12, 2024 •

edited

Loading