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

Dispatcher for template parameters of BuildHist Kernels #8259

Merged
merged 18 commits into from
Oct 6, 2022

Conversation

razdoburdin
Copy link
Contributor

Hi, I continue working on optimizations of histogram building. In this PR I suggest to refactor dispatching of runtime flags into the template parameters.
Currently histogram building functions require 3-4 template parameters. In the next optimization step few more template parameters will required. It will make template parameters list not a good looking. Moreover it will require more dispatcher functions.
For this reasons, I suggest to use a special dispatcher class, being described in this PR.

@razdoburdin razdoburdin changed the title Optimization/buildhist/dispatcher Dispatcher for template parameters of BuildHist Kernels Sep 22, 2022
@RAMitchell
Copy link
Member

Does std::variant do what you want? We don't have C++17 yet but we could either upgrade or just implement a common::variant and switch it to the standard library when we do upgrade.

@razdoburdin
Copy link
Contributor Author

I don't think so.
std::variant will allow to store objects of various classes (various classes from GHistBuildingManager template, in my case). But here are at least two problems:

  1. One needs to manually determine possible classes for std:variant (std::variant<Class1, Class2, ..., ClassN> var). Number of such classes grows exponentially with increasing the number of template parameters.
  2. One still needs some way to convert runtime variables to specific class of GHistBuildingManager template. But if such a way exist, it is easier to use this class as a template parameter.

Did I catch your idea correctly?

@RAMitchell
Copy link
Member

More specifically std::variant in conjunction with std::visit performs dispatching like you have in your code:

GHistBuildingManager<any_missing>::DispatchAndExecute(
    {first_page, read_by_column || force_read_by_column, bin_type_size},
    [&](auto t) {
      using BuildingManager = decltype(t);
      BuildHistDispatch<BuildingManager>(gpair, row_indices, gmat, hist);
    });

@razdoburdin
Copy link
Contributor Author

You are right, this part is quite similar to std::visit.
However, one still need to write something like:

std::variant<GHistBuildingManager<any_missing, true,  true,  uint8_t>,
              GHistBuildingManager<any_missing, true,  true,  uint16_t>,
              GHistBuildingManager<any_missing, true,  true,  uint32_t>,
              GHistBuildingManager<any_missing, true,  false, uint8_t>,
              GHistBuildingManager<any_missing, true,  false, uint16_t>,
              GHistBuildingManager<any_missing, true,  false, uint32_t>,
              GHistBuildingManager<any_missing, false, true,  uint8_t>,
              GHistBuildingManager<any_missing, false, true,  uint16_t>,
              GHistBuildingManager<any_missing, false, true,  uint32_t>,
              GHistBuildingManager<any_missing, false, false, uint8_t>,
              GHistBuildingManager<any_missing, false, false, uint16_t>,
              GHistBuildingManager<any_missing, false, false, uint32_t>> building_manager;

to define std::variant. And amount of template parameters here grow twice if someone will add one more flag.
Also initialization of this std::variant could be quite tricky.

Do you see something here, that I have missed?

@RAMitchell
Copy link
Member

RAMitchell commented Sep 23, 2022

Here is how we can instantiate many booleans into template functions without writing too much code:

In your case you have the enum as well, so you can wrap SomeFunction inside the dispatcher for this enum.

#include <iostream>
#include <variant>

std::variant<std::false_type,std::true_type> to_variant(bool b)
{
    if(b){
        return std::true_type{};
    }else{
        return std::false_type{};
    }
}

template <bool is_missing, bool something_else>
void SomeFunction(){
    std::cout << is_missing << std::endl;
    std::cout << something_else << std::endl;
}

int main()
{
    bool runtime_a = true;
    bool runtime_b = false;
    std::visit([](auto a, auto b) {
            SomeFunction<a, b>();
        }, to_variant(runtime_a), to_variant(runtime_b));

}

Given that we don't have C++17 I leave it up to you what you want to do.

@razdoburdin
Copy link
Contributor Author

razdoburdin commented Sep 27, 2022

Got it! The solution based on std::visit is really much less verbose. But adding of a common::variant can be quite tricky and take a time. Can the current realization be added first? One can change it to std::variant after the C++17 will be supported.

@razdoburdin
Copy link
Contributor Author

@RAMitchell What is your decision about this PR?

@razdoburdin
Copy link
Contributor Author

Hi @trivialfis,
could you please describe the merging process?
This PR is already approved but still not merged. Are there any actions required from my side to speed up the process?

@hcho3 hcho3 merged commit c24e9d7 into dmlc:master Oct 6, 2022
@hcho3
Copy link
Collaborator

hcho3 commented Oct 6, 2022

Merging this, since all CPU tests passed.

@hcho3 hcho3 mentioned this pull request Oct 6, 2022
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.

3 participants