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

Issue 7047 optional jemalloc #7424

Merged
merged 29 commits into from
Nov 13, 2024

Conversation

amsmota
Copy link
Contributor

@amsmota amsmota commented Aug 4, 2024

PR description

Fixed Issue(s)

Fixes #7047

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@amsmota amsmota mentioned this pull request Aug 4, 2024
@usmansaleem usmansaleem self-requested a review August 11, 2024 23:12
Copy link
Member

@usmansaleem usmansaleem left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
@fab-10
Copy link
Contributor

fab-10 commented Aug 12, 2024

@amsmota @usmansaleem I just had a different idea about how to implement, that is more in line with the requirements. Basically we have the besu-untuned script that seems to perfectly fit the purpose of having a vanilla Besu command, without any special options enforced, that can be used to perform any kind of customization, so I would rather suggest to keep besu command as is, and remove all the jemalloc related code from besu-untuned.

@amsmota
Copy link
Contributor Author

amsmota commented Aug 13, 2024

@amsmota @usmansaleem I just had a different idea about how to implement, that is more in line with the requirements. Basically we have the besu-untuned script that seems to perfectly fit the purpose of having a vanilla Besu command, without any special options enforced, that can be used to perform any kind of customization, so I would rather suggest to keep besu command as is, and remove all the jemalloc related code from besu-untuned.

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
https://wiki.hyperledger.org/display/BESU/Reduce+Memory+usage+by+choosing+a+different+low+level+allocator

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.

@fab-10
Copy link
Contributor

fab-10 commented Aug 13, 2024

@amsmota besu-untuned is a start script, you can find it in the bin dir, that is like the besu standard script, but hasn't any preset configuration for Java memory, so you can use it to test your own setup, for example change GC, or other flags, that is exactly what you want to test a custom setup, to see if it is better than the proposed one.
So removing the preset of the allocator from besu-untuned goes in the right directing of giving the freedom to the user to selected whichever allocator he want, and should have been already the case.
Do not worry, I can pick this task if you haven't time now, and thanks for raising that requirement.

@amsmota
Copy link
Contributor Author

amsmota commented Aug 13, 2024

@amsmota besu-untuned is a start script, you can find it in the bin dir

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.

@fab-10
Copy link
Contributor

fab-10 commented Aug 13, 2024

To use jemalloc with the besu-untuned script (when it will be changed), you just need to run

export LD_PRELOAD=libjemalloc.so; bin/besu-untuned

@amsmota
Copy link
Contributor Author

amsmota commented Aug 13, 2024

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 Native.load won't load, it loads it but just for Java internal calls, but it does not influence the behavior of memory allocation.

I also tested with LD_PRELOAD but removing the Native.load part, it works fine but I have no way to confirm that jemalloc is loaded. However, I have no reason to believe it dosen't, so I assume it is loaded.

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?

@amsmota
Copy link
Contributor Author

amsmota commented Aug 13, 2024

To use jemalloc with the besu-untuned script (when it will be changed), you just need to run

export LD_PRELOAD=libjemalloc.so; bin/besu-untuned

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]>
Copy link
Member

@usmansaleem usmansaleem left a 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.

@amsmota
Copy link
Contributor Author

amsmota commented Aug 17, 2024

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

if [ -z "$BESU_USING_JEMALLOC" ] || { [ -n "$BESU_USING_JEMALLOC" ] && [ "$BESU_USING_JEMALLOC" != "FALSE" ] && [ "$BESU_USING_JEMALLOC" != "false" ]; }; then
    if [ "\$darwin" = "false" -a "\$msys" = "false" ]; then
      # check if jemalloc is available
      TEST_JEMALLOC=\$(LD_PRELOAD=libjemalloc.so sh -c true 2>&1)

      # if jemalloc is available the output is empty, otherwise the output has an error line
      if [ -z "\$TEST_JEMALLOC" ]; then
        export LD_PRELOAD=libjemalloc.so
      else
        # jemalloc not available, as fallback limit malloc to 2 arenas
        export MALLOC_ARENA_MAX=2
      fi
    fi
fi

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.

  • What went wrong:
    Execution failed for task ':evmToolStartScripts'.

Could not write to file 'C:\Users\anton\Development\OSPO\besu\build\scripts\evmtool'

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.

@fab-10
Copy link
Contributor

fab-10 commented Aug 30, 2024

@amsmota use this code, since this is a template for the shell script, you need to escape $ and it is possible to simplify the test expression

if [ "\$darwin" = "false" -a "\$msys" = "false" ]; then
    if [ "\$BESU_USING_JEMALLOC" != "FALSE" -a "\$BESU_USING_JEMALLOC" != "false" ]; then
      # check if jemalloc is available
      TEST_JEMALLOC=\$(LD_PRELOAD=libjemalloc.so sh -c true 2>&1)

      # if jemalloc is available the output is empty, otherwise the output has an error line
      if [ -z "\$TEST_JEMALLOC" ]; then
        export LD_PRELOAD=libjemalloc.so
        export BESU_USING_JEMALLOC=true
      else
        # jemalloc not available, as fallback limit malloc to 2 arenas
        export MALLOC_ARENA_MAX=2
      fi
    fi
fi

@amsmota
Copy link
Contributor Author

amsmota commented Oct 30, 2024

@usmansaleem, @fab-10, I deleted everithing from my WSL, both the code and the ~/.gradle. I then run a ./gradlew build but it fails:

Task :evmToolStartScripts FAILED

FAILURE: Build failed with an exception.

  • What went wrong:
    Execution failed for task ':evmToolStartScripts'.

Could not write to file '/home/amsmota/Development/besu/build/scripts/evmtool'.

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.

@amsmota
Copy link
Contributor Author

amsmota commented Oct 30, 2024

Could not write to file '/home/amsmota/Development/besu/build/scripts/evmtool'.

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...

FAILURE: Build failed with an exception.

  • What went wrong:
    Gradle build daemon disappeared unexpectedly (it may have been killed or may have crashed)

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.

Java HotSpot(TM) 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended

Cheers.

@amsmota
Copy link
Contributor Author

amsmota commented Oct 31, 2024

@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:

NO Jemalloc Installed / NO BESU_USING_JEMALLOC
jemalloc library not found, memory usage may be reduced by installing it

NO Jemalloc Installed / BESU_USING_JEMALLOC = false
BESU_USING_JEMALLOC is present but is not set to true, jemalloc library not loaded

NO Jemalloc Installed / BESU_USING_JEMALLOC = true
BESU_USING_JEMALLOC is present but we failed to load jemalloc library to get the version

WITH Jemalloc Installed / NO BESU_USING_JEMALLOC
jemalloc: 5.2.1-0-gea6b3e973b477b8061e0076bb257dbd7f3faa756

WITH Jemalloc Installed / BESU_USING_JEMALLOC = false
BESU_USING_JEMALLOC is present but is not set to true, jemalloc library not loaded

WITH Jemalloc Installed / BESU_USING_JEMALLOC = true
jemalloc: 5.2.1-0-gea6b3e973b477b8061e0076bb257dbd7f3faa756

I still have some work to do, namelly

  • fix some jemalloc tests that are now failing
  • change those outputs above to print BESU_USING_JEMALLOC in lowers (I don't like the way they look in capitals in the log)
    "besu_using_jemalloc is present but we failed to load jemalloc library to get the version"
  • make a few changes in the script

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.

@amsmota
Copy link
Contributor Author

amsmota commented Nov 3, 2024

@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:

  1. NO Jemalloc Installed / NO BESU_USING_JEMALLOC
  2. NO Jemalloc Installed / BESU_USING_JEMALLOC = false
  3. NO Jemalloc Installed / BESU_USING_JEMALLOC = true
  4. WITH Jemalloc Installed / NO BESU_USING_JEMALLOC
  5. WITH Jemalloc Installed / BESU_USING_JEMALLOC = false
  6. WITH Jemalloc Installed / BESU_USING_JEMALLOC = true

and the results in the logs shloud be

  1. jemalloc library not found, memory usage may be reduced by installing it
  2. besu_using_jemalloc is present but is not set to true, jemalloc library not loaded
  3. besu_using_jemalloc is present but we failed to load jemalloc library to get the version
  4. jemalloc: 5.2.1-0-gea6b3e973b477b8061e0076bb257dbd7f3faa756
  5. besu_using_jemalloc is present but is not set to true, jemalloc library not loaded
  6. jemalloc: 5.2.1-0-gea6b3e973b477b8061e0076bb257dbd7f3faa75

(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.

@amsmota
Copy link
Contributor Author

amsmota commented Nov 9, 2024

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.

@usmansaleem usmansaleem dismissed their stale review November 10, 2024 23:49

the changes were applied.

@fab-10 fab-10 enabled auto-merge (squash) November 11, 2024 09:50
@amsmota amsmota closed this Nov 12, 2024
auto-merge was automatically disabled November 12, 2024 20:05

Pull request was closed

@usmansaleem usmansaleem reopened this Nov 13, 2024
@usmansaleem
Copy link
Member

@amsmota You accidently closed it, I've reopened it and enabled automerge. What you need to do is merge the main branch back onto your branch. Once you have up-to-date changes from main branch, due to automerge enabled, it will merge once all tests pass.

@usmansaleem usmansaleem enabled auto-merge (squash) November 13, 2024 04:11
@amsmota
Copy link
Contributor Author

amsmota commented Nov 13, 2024

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.

@usmansaleem
Copy link
Member

@amsmota you don't need to "close" PR, once PR is merged to main, its status will automatically changed.

@usmansaleem usmansaleem disabled auto-merge November 13, 2024 08:29
@usmansaleem usmansaleem enabled auto-merge (squash) November 13, 2024 08:30
@amsmota
Copy link
Contributor Author

amsmota commented Nov 13, 2024

@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?

@usmansaleem
Copy link
Member

@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 :)

@amsmota
Copy link
Contributor Author

amsmota commented Nov 13, 2024

@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 👍

@usmansaleem usmansaleem merged commit c15afb9 into hyperledger:main Nov 13, 2024
43 checks passed
@amsmota
Copy link
Contributor Author

amsmota commented Nov 13, 2024

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... 💯

JanetMo pushed a commit to JanetMo/besu that referenced this pull request Nov 17, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optionally load jemalloc
3 participants