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

aws: fork mbedtls 2.25.0 base64 utility to avoid 2.26.0+ performance hit #4422

Merged
merged 4 commits into from
Feb 11, 2022

Conversation

matthewfala
Copy link
Contributor

Migrate to performant 2.25.0-mbedtls base64 implementation.

Tested via unit tests + 1 end to end test
Sent logs to firehose via the out_kinesis_firehose plugin which utilizes the implementation.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • [N/A] Debug log output from testing the change
  • [N/A] Attached Valgrind output that shows no leaks or memory corruption was found (No memory allocations)

Documentation

  • Documentation required for this feature

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@matthewfala matthewfala force-pushed the base64-aws branch 2 times, most recently from 2b7baf3 to 0434352 Compare December 10, 2021 01:21
PettitWesley
PettitWesley previously approved these changes Dec 13, 2021
Copy link
Contributor

@PettitWesley PettitWesley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edsiper I'd like your review/approval on this as well.

@edsiper
Copy link
Member

edsiper commented Dec 13, 2021

@edsiper
Copy link
Member

edsiper commented Dec 13, 2021

Why not to make it generic use instead of just aws scoped? I think it could be useful as flb_base64.c alone

@matthewfala
Copy link
Contributor Author

Strange how it passes on some operating systems. Grateful for the crossplatform unit test runner. I'll take a look. I can refactor this to flb_base64

Since I did some refactoring to the aws unit test structure for the base64 test, I might switch to an aws placeholder test that noops, so future tests to aws can be added easily if that is alright.

@matthewfala
Copy link
Contributor Author

@PettitWesley Could you take a look at the changes? 3c08a68

I think I resolved the problem with the testcase, it may have just been a lack of a null character when calling strlen. I initialize the array to 0 now in the decode test case.

@PettitWesley
Copy link
Contributor

@matthewfala CI says the tests are still failing? I am good to approve/merge once everything passes.

@matthewfala
Copy link
Contributor Author

Need to refactor the commit messages, then this should be done

PettitWesley
PettitWesley previously approved these changes Dec 16, 2021
@matthewfala
Copy link
Contributor Author

@edsiper Could you please take a look at this when you get the chance?

@nokute78
Copy link
Collaborator

@matthewfala @PettitWesley @krispraws
Before revewing

@edsiper
Copy link
Member

edsiper commented Jan 17, 2022

thanks @nokute78 for the hints about mbedtls, I will make sure to upgrade in the 1.8 branch

@matthewfala
Copy link
Contributor Author

Hi @nokute78,
Actually, could you let me know if this pr will be considered? Then I'll fix the conflicts.

According to Ramya's testing as described in the following issue:
#4110
The performance hits of Mbedtls's new base64 function is severe, causing significant drop in throughput in our plugin that uses mbedtls.

It appears the newest base64 implementation is still slower than the older implementation as described by the changelog message in ChangeLog.d/base64-ranges.txt

Changes
   * Improve the performance of base64 constant-flow code. The result is still
     slower than the original non-constant-flow implementation, but much faster
     than the previous constant-flow implementation. Fixes #4814.

Would forking be considered above just upgrading mbedtls?

@lecaros lecaros added this to the Fluent Bit v1.8.12 milestone Jan 21, 2022
@nokute78
Copy link
Collaborator

@matthewfala In my opinion, forking can be a risk and take a porting cost. e.g. Watching upstream and merging patches.
If the performance using mbedtls v2.28 is not acceptable for user/developer, we should merge this PR as an interim patch.
If it is not, we need to upgrade mbedtls.

How much slower is it using mbedtls v2.28 ?

@matthewfala
Copy link
Contributor Author

Thank you @nokute78. I like the interim patch idea. I'll fix the merge conflicts. We can assess the speed difference after, though I am not familiar with profiling, so I'll either learn or ask someone else on the team do help.

@matthewfala
Copy link
Contributor Author

@nokute78 Could you take a look? Resolved merge conflicts

Copy link
Collaborator

@nokute78 nokute78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add test case to compare performance between flb_base64_* and mbedtls_base64_* ?
The test fails when mbedtls_base64_* are faster than flb_base64_* or the performance of mbedtls_base64_* becomes acceptable.

We can detect it is the time to drop the interim patch and back to mbedtls.

@matthewfala matthewfala requested a review from nokute78 February 1, 2022 18:06
Copy link
Collaborator

@nokute78 nokute78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewfala Thank you for updating.
LGTM.

@matthewfala
Copy link
Contributor Author

@nokute78 YW. Could you give me an eta on when this will be merged?

@nokute78
Copy link
Collaborator

@matthewfala I can't merge since "Merging can be performed automatically with 1 approving review." .
@edsiper Could you merge this ?

@edsiper edsiper merged commit b28bfa4 into fluent:master Feb 11, 2022
@edsiper
Copy link
Member

edsiper commented Feb 11, 2022

@matthewfala would you please submit a PR for 1.8 branch ?

@edsiper edsiper added backport to v1.8.x Used to tag items that must be backported to such version. and removed docs-required labels Feb 11, 2022
@matthewfala
Copy link
Contributor Author

Definitely! Will publish a pr to 1.8 today.

@matthewfala matthewfala mentioned this pull request Feb 11, 2022
6 tasks
@matthewfala
Copy link
Contributor Author

Here is the backport PR!: #4801

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport to v1.8.x Used to tag items that must be backported to such version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants