-
Notifications
You must be signed in to change notification settings - Fork 871
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
Issue 7047 optional jemalloc #7424
Issue 7047 optional jemalloc #7424
Conversation
Signed-off-by: Antonio Mota <[email protected]>
Signed-off-by: Antonio Mota <[email protected]>
… after review Signed-off-by: Antonio Mota <[email protected]>
… entry to CHANGELOG Signed-off-by: Antonio Mota <[email protected]>
…i/besu into issue-7047-optional-jemalloc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment please.
@amsmota @usmansaleem I just had a different idea about how to implement, that is more in line with the requirements. Basically we have the |
Hi, TBH I don't know what that besu-untuned is, do you have any link to docs or something? Anyway, besu already supports Jemalloc, right? They did intensive tests with several mallocs and they found out Jemalloc is the one that gives better performance, an it is part of the product now. Are they supposed to remove it from besu main? If not this PR fits the bill, it contains minimal changes to it and it works fine. So I would like to go ahead with this PR. https://wiki.hyperledger.org/display/BESU/2024+-+Besu+Performance+Improvements+since+the+Merge On a personal note, I won't have much time to contribute to Besu since I moved from Blockchain to GenAI and I have all my time assigned to it, that's why I want to wrap up my 2 PRs as fast as I can... Cheers. |
@amsmota |
Oh, I saw it now... I thougth it was a fork of the main project or so... I never care to look at that folder after installDist... So with that "untuned" version it make all sense to remove any reference to Jemalloc as you say, and let it use the OS default Malloc and let the users/developers to do what they think it's better. However, the fact is that the main Besu already support that, for what I see since 22.7.0-RC2, so I think it's still good to provied this change to the "general" population that uses Besu out-of-the-box. In our case this concerns more the Prod deployment, because we need to quickly restart a deployment without Jemalloc if there are failures, and the current code will always load Jemalloc if it is available... In the meantime I updated my branch to the latest main, I think I'll have to test everithing again... Cheers. |
To use
|
Ok, guys, I updated and build my branch, I uninstalled jemalloc, tested with BESU_USING_JEMALLOC true and false and without it, all messages appeared correctly, I installed jemalloc again and tested with BESU_USING_JEMALLOC true and false and without it and all messages appeared correctly too. I also tested removing all LD_PRELOAD from the scripts and you guys are right, it is needed to load jemalloc, my bad... It's not that I also tested with LD_PRELOAD but removing the So what I am going to do is to remove the BESU_USING_JEMALLOC from the script, since it does not exist it will load jemalloc if exists any way, and let users set BESU_USING_JEMALLOC if and as they need it... What do youse guys think? |
Yes, the only issue is that there will be nothing in the logs saying it is using it. But then again, whoever adds the LD_PRELOAD will know. It will be a nice-to-have to log some info, but will not change any behaviour... |
Signed-off-by: Antonio Mota <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The detectJeMalloc
in ConfigurationOverviewBuilder
can be simplified. It can also be renamed to detectAndLogJeMallocVersion
to make the intent more clear.
besu/src/main/java/org/hyperledger/besu/cli/ConfigurationOverviewBuilder.java
Show resolved
Hide resolved
Guys, I really got this wrong, I assumed things that I shouldn't have assumed, like assuming that LD_PRELOAD and Native.load would both load the lib, which is true but they are used for diferent ends. Thank you guys for correcting me. However, my goal of be able to load or not load Jemmaloc when it does exist it's not achivable with any of the solutions I gave, so now what I think it should be done to achive that is change the unix script to be like
or probably with that IF just before the export LD_PRELOAD. I did this change but I'm not able to test it, for some starnge reason both in Win and WSL I'm getting this error that I never saw, never happened before, and googling gives me no idea. I tried the obvious, to delete the entire build dir and add 777 access to subdirs, but it keeps on giving that error.
Anyway, I'll commit in case any of youse are able to test... I'll be able to do some work/testing tomorrow and Monday, probably I'm going to delete my local and checkout again, and test properly. And I'll check your latest comment too. Thank you guys for all the help. Cheers. |
Signed-off-by: Antonio Mota <[email protected]>
@amsmota use this code, since this is a template for the shell script, you need to escape
|
@usmansaleem, @fab-10, I deleted everithing from my WSL, both the code and the ~/.gradle. I then run a ./gradlew build but it fails:
I am running with my user user, not with sudo, maybe I should try to do it as su? Anyway, did youse saw this issue before and/or know how to fix it? Cheers. |
Well, I fixed this one 👍 , which is good because it was I that caused...... 👎. And BTW the fix was actually the @fab-10 code from #7424 (comment). I could swear I had done that at some point but it seems I didn't... But anyway, I have another error, one that I had before when working in #6965 and I couldn't solve, and it is is a linux error too...
This doesen't happen building on windows, only in WSL. There too some lines that I don't know of are related, I'll try to investigate that.
Cheers. |
@fab-10 , @usmansaleem I finnally was able to build my branch and tested it and IT'S ALL GOOD !!!! This are the tests I did and the results output in the log:
I still have some work to do, namelly
A little concern I had is that I installed jemalloc on Ubuntu using the libjemalloc2 lib, that installs it as libjemalloc.so.2 . I had to rename it libjemalloc.so to make it compatible with the script, a note somewhere should warn about this... Anyhow, by tomorrow or Saturday I should have this pushed to the PR. Cheers. |
@fab-10 , @usmansaleem et all, I finally finsihed my changes, the failing test is now running and the other small changes done too. I tested again on my WSL and it all seems right. Can youse please review the changes and hopefully test and give me some feedback? Please note that the expected scenarios that I tested are:
and the results in the logs shloud be
(jemalloc version can be diferent, of course) Guys, I can't thank you enough for your help, If you ever come to Dublin (In Ireland, not Ohio, believe me or not Facebook already tagged me in Ohio a few times!!!) I will gladly offer youse a couple of Guinnessess (it's a non written rule here that youse shall not drink only one).... Cheers. |
besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java
Outdated
Show resolved
Hide resolved
Hi @fab-10 , I see you aproved my last changes, can you tell me what are the next steps for me to be able to close this PR? @usmansaleem I see one requested change made by you, can you approve/dismiss it? Thank you guiys. |
@amsmota You accidently closed it, I've reopened it and enabled automerge. What you need to do is merge the |
Hi @usmansaleem, thanks for your quick reply. I just did that merge so I'll wait for the tests to be run and close the PR. Thank you again for you colaboration, it is greatly appreciated. Cheers. |
@amsmota you don't need to "close" PR, once PR is merged to main, its status will automatically changed. |
So I can close the PR now? |
@amsmota As I said, if you close the PR it won't merge. As we have enabled "auto merge", it will merge on its own. Just sit back and relax :) |
Got it 👍 |
Guys, @usmansaleem and @fab-10 and everybody that helped with sugestions and comments my sincere gratitude, it was a task way more dificult than I initially supposed and I wouldn't be sucesffull in doing it without yours support. It seems that saying "standing in the shoulder of giants" it's actually true... Well, "may the road rise to meet youse" as they say here in Ireland... 💯 |
* Implementing Issue hyperledger#7047 - Optionally load jemalloc Signed-off-by: Antonio Mota <[email protected]> * Implementing Issue hyperledger#7047 - Optionally load jemalloc Signed-off-by: Antonio Mota <[email protected]> * Implementing Issue hyperledger#7047 - Optionally load jemalloc: fixes after review Signed-off-by: Antonio Mota <[email protected]> * Implementing Issue hyperledger#7047 - Optionally load jemalloc: added entry to CHANGELOG Signed-off-by: Antonio Mota <[email protected]> * Changes after review Signed-off-by: Antonio Mota <[email protected]> * Added env var check in unix script Signed-off-by: Antonio Mota <[email protected]> * Improved code and script, build and tested Signed-off-by: amsmota <[email protected]> * Improved code and script, build and tested Signed-off-by: amsmota <[email protected]> --------- Signed-off-by: Antonio Mota <[email protected]> Signed-off-by: amsmota <[email protected]> Signed-off-by: Marlene Marz <[email protected]>
PR description
Fixed Issue(s)
Fixes #7047
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests