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

Prepare a doc to help ebable autofdo for nebulagraph #4983

Closed
wants to merge 1 commit into from

Conversation

lipzhu
Copy link
Contributor

@lipzhu lipzhu commented Dec 4, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

#4636

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

@lipzhu
Copy link
Contributor Author

lipzhu commented Dec 5, 2022

@wey-gu Do you want to build a process to generate a profile regularly and provide an option for users to enable the AutoFDO when building from source?

@yixinglu yixinglu requested a review from dutor December 6, 2022 03:57
@wey-gu
Copy link
Contributor

wey-gu commented Dec 16, 2022

Oops, Sorry @lipzhu I missed this mention for a month, shame for this.

Thanks @xtcyclist @yixinglu btw, what do you guys think of this?

cc @dutor @critical27 @Sophie-Xie

@lipzhu
Copy link
Contributor Author

lipzhu commented Dec 17, 2022

@wey-gu Its my apologies that delay the document so long. Hope you still have interests about this :)

Copy link
Contributor

@dutor dutor left a comment

Choose a reason for hiding this comment

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

Thanks for this experiment and contribution.

I wonder if there are ways to automate the autofdo build process. After all, we have to integrate it to our standard release workflow.

add_compile_options(-Wnon-virtual-dtor)
add_compile_options(-Woverloaded-virtual)
add_compile_options(-Wignored-qualifiers)
+add_compile_options(-fauto-profile=~/fbdata.afdo)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to customize the build with some CMake option rather than such an ad-hoc change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. We can add an default option if the user want to use the default profile provided by community.
And also provide an option to the users who want to build their own profile data based on their real production environment.

@lipzhu
Copy link
Contributor Author

lipzhu commented Dec 23, 2022

Thanks for this experiment and contribution.

I wonder if there are ways to automate the autofdo build process. After all, we have to integrate it to our standard release workflow.

My suggestion is to integrate the work into the community standard build workflow, the community refreshed the profile periodically.
Just as chromium community did https://chromium.googlesource.com/chromiumos/chromite/+/master/cbuildbot/afdo.py.

@lipzhu lipzhu requested a review from dutor December 23, 2022 09:35
@dutor
Copy link
Contributor

dutor commented Feb 1, 2023

Sorry about the late reply.

It takes a lot of effort to integrate this to our build process. But I think the effort is worthy, we will consider to utilize it seriously in the develop plan. Before that, I sugguest we could add this doc to our official docment as an alternative build method, instead of putting it in our source tree.

@wey-gu What do you think?

@wey-gu
Copy link
Contributor

wey-gu commented Feb 1, 2023

It takes a lot of effort to integrate this to our build process. But I think the effort is worthy, we will consider to utilize it seriously in the develop plan.

Wow, looking forward to it!

Before that, I sugguest we could add this doc to our official docment as an alternative build method, instead of putting it in our source tree.

Agreed, this will enable those who would like to benefit from autofdo before we bring it to the build process.

@whitewum what do you think, please? could we somehow put the guide of this PR to docs in a proper chapter and co-author @lipzhu for the docs PR?

@whitewum
Copy link
Contributor

whitewum commented Feb 2, 2023

It takes a lot of effort to integrate this to our build process. But I think the effort is worthy, we will consider to utilize it seriously in the develop plan.

Wow, looking forward to it!

Before that, I sugguest we could add this doc to our official docment as an alternative build method, instead of putting it in our source tree.

Agreed, this will enable those who would like to benefit from autofdo before we bring it to the build process.

@whitewum what do you think, please? could we somehow put the guide of this PR to docs in a proper chapter and co-author @lipzhu for the docs PR?

Sure. Great work.

I think currently we can add a link in the compile part of the manual to this pr, and mention this is an alternative.

@lipzhu
Copy link
Contributor Author

lipzhu commented Feb 2, 2023

It takes a lot of effort to integrate this to our build process. But I think the effort is worthy, we will consider to utilize it seriously in the develop plan. Before that, I sugguest we could add this doc to our official docment as an alternative build method, instead of putting it in our source tree.

Cool~ looking forward to the official release version of NebulaGraph with AFDO.

@wey-gu
Copy link
Contributor

wey-gu commented Feb 9, 2023

@dutor I would like to suggest merging this as doc in the core repo as is(or in some other path), now, what do you think, please?

@lipzhu
Copy link
Contributor Author

lipzhu commented Feb 17, 2023

Sorry about the late reply.

It takes a lot of effort to integrate this to our build process. But I think the effort is worthy, we will consider to utilize it seriously in the develop plan. Before that, I sugguest we could add this doc to our official docment as an alternative build method, instead of putting it in our source tree.

@wey-gu What do you think?

BTW, @dutor do you mind sharing the performance data about how much afdo can gain from your tests?

@lipzhu lipzhu closed this Feb 20, 2023
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.

4 participants