Skip to content
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.

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

Change findblocks into promise have a sense? #3218

Closed

Conversation

sefirosweb
Copy link
Contributor

Hi guys, I have been noticed that my bot sometimes freezes, I have detected that if I run findblock to search in a large radius, this causes that the bot freeze until the search is finished,

I think it would be a good idea change the method to a promise, and give it a short wait for the bot to exchange the action processes with the search and this not freeze the bot,

The change is simple, which will cause users to have to adapt their code to a promise

Obviously the performance is worse; the same test for "normal"/"promise" is 3-4 times slower,
What this slowness in my opinion compensates for the bot being much "fresher" in these actions without blocking,

image

The important code are only in block.js
Change findblocks into promise have a sense?

@rom1504
Copy link
Member

rom1504 commented Oct 29, 2023 via email

@extremeheat
Copy link
Member

Also, yielding to the event loop inside of any hot/iterative code is really bad as that prevents any JIT optimizations, on top of the event loop overhead (copying memory, waiting, etc). If it's really necessary to split up work, you would need to find a better algorithm to segment the data and yield as little as possible (maybe a few times depending on search area) to make it optimal.

I think a better solution would be to profile the existing code and see if synchronous optimizations could be made (reducing copies, type conversions, checks, etc). As of right now, it seems findBlock() is running through every block in the box, when it could be exiting early on the first match. That's an easy optimization.

@sefirosweb
Copy link
Contributor Author

I don't think it can help much in optimizing the code, I don't understand it very well, and when I want to see the variable types I see that many of them are not complete.

image

If you want I can make a new method "searchBlock" & "searchBlocks" that use a common code or most of it

@extremeheat
Copy link
Member

I don't think it can help much in optimizing the code

It will, pushing things to do later will make things slower, not faster. It will still block your program, including timers. The only way to not block aside a worker is to do less operations or more efficient ones.

I don't understand it very well, and when I want to see the variable types I see that many of them are not complete.

Use control + f to search the codebase, it's not very big or complicated code, everything is defined in a few lib/plugins files.

If you want I can make a new method "searchBlock" & "searchBlocks" that use a common code or most of it

Echo my previous comment: As of right now, it seems findBlock() is running through every block in the box, when it could be exiting early on the first match. That's an easy optimization.

@sefirosweb
Copy link
Contributor Author

sefirosweb commented Oct 29, 2023

I'm on it =)

It will, pushing things to do later will make things slower, not faster. It will still block your program, including timers. The only way to not block aside a worker is to do less operations or more efficient ones.

Yes and not, yes is more slower, but the bot dosent freeze on to do this operation, (of course this affect to the rest of bot while are executing )

anyways i try to see if it can also be improved without promises

@szdytom
Copy link
Contributor

szdytom commented Nov 1, 2023

Possibly related to #3216.

@extremeheat
Copy link
Member

anyways i try to see if it can also be improved without promises

One of the best ways to make things faster is by minimizing memory allocations/copies, especially in hot code. Simplifying the code helps, but be cautious here since less code doesn't mean faster, if that "less code" is just an abstraction for really complex code under the hood. PrismarineJS/prismarine-physics@d29774a is a good example for that, as lots of garbage from temporary objects would be created in hot code.

Also, moving the world data and these findBlock methods to prismarine-world per #334 would make it easier to remove where the bottlenecks are in the current code.

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

ci is failing

@sefirosweb
Copy link
Contributor Author

I tried to improve the optimization but without results..

Finally are you interested to merge this pr?

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

yeah actually reading the previous discussion it seems this PR is not really helping right? maybe we should just close it?

@sefirosweb
Copy link
Contributor Author

There are contrary opinions, I think so, but I do not have the knowledge to improve performance, if perhaps it is not interesting we can close the pr

@rom1504
Copy link
Member

rom1504 commented Dec 30, 2023

ok

@rom1504 rom1504 closed this Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants