Issue 7047 optional jemalloc

[amsmota]

PR description

Fixed Issue(s)

See #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

[usmansaleem]

See the comment please.

[fab-10]

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

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

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

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

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]

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]

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

NA

[usmansaleem]

The detectJeMalloc in ConfigurationOverviewBuilder can be simplified. It can also be renamed to detectAndLogJeMallocVersion to make the intent more clear.

[usmansaleem]

Since you are removing BESU_USING_JEMALLOC from the start script, the detectJemalloc can be simplified as:

  private void detectJemalloc(final List<String> lines) {
    try {
      final String version = PlatformDetector.getJemalloc();
      lines.add("jemalloc: " + version);
    } catch (final Throwable throwable) {
      logger.info(
              "jemalloc library not found, memory usage may be reduced by installing it");
    }
  }

[usmansaleem]

This changelog entry needs to change specially if we simplify detectMalloc method. This can be rephrased similar to: "Simplified logging of jemalloc library at Besu startup".

[amsmota]

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]

@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

[usmansaleem]

@amsmota See #7424 (comment) please.