Fix single row prediction performance in a multi-threaded environment

[Ten0]

Fixes #6021

• Store all resources that are reused across single-row predictions in the dedicated SingleRowPredictor (aka FastConfig)
• Use that instead of resources in the Booster when doing Fast™ single row predictions to avoid having to lock the Booster exclusively.

[Ten0]

Workflows run would help :) (not sure what is currently using this and should be tested)

[Ten0]

@microsoft-github-policy-service agree

[jameslamb]

You don't need to force-push here. All commits will be squashed on merge.

[Ten0]

You don't need to force-push here. All commits will be squashed on merge.

Thanks! :) That was just me organizing my work though (I wanted this commit specifically to be a single one and working in case it would end up being reverted)

[jameslamb]

@shiyu1994 or @guolinke could you please help with a review on this one?

And @AlbertoEAF if you have time?

[AlbertoEAF]

@Ten0 thanks for contributing! Since we're adding capability for it, can you add a test to our C++ test suite showing multiple predictors using the fast API and sharing the booster?

[Ten0]

So I've finished my benchmarks and it turns out:

• I can see the expected performance improvements of x N_CPUs performance
• The sub-optimal locking has no impact
• I was afraid that memory barriers due to the read lock might be noticeable, so I thought a bit unfortunate that we would have to take the shared read lock for every prediction. I was originally considering holding the shared lock for the entire lifetime of the SingleRowPredictor, but as it turned out some implementations using the C library (notably R) would rely on being able to update the Booster after constructing a SingleRowPredictor, so that would cause a deadlock. (f41755b). As it turns out, this shows no impact in my benchmarks, so I'm going to be reasonably happy with leaving it as is (that is, with taking a shared lock for every single row prediction, as is currently implemented).
  

[stonebrakert6]

Thanks a lot @Ten0 for this commit. I am a new user of lightGBM(need to mention this so that you don't confuse me with a veteran).
So, IIRC, the usage now would be something similar to this

1. BoosterHandle is created once and is then shared among all threads.
2. Each thread has its own copy of FastConfigHandle and then calls LGBM_BoosterPredictForMatSingleRowFastInit to initialize its private(non-shared) FastConfigHandle using the BoosterHandle created in step 1.
3. Each thread can now call LGBM_BoosterPredictForMatSingleRowFast(passing in its FastConfigHandle) to make predictions.

Is this understanding correct?

[Ten0]

Each thread has its own copy of FastConfigHandle

Of BoosterHandle but I think that's what you meant. Otherwise yes that's exactly it.

[stonebrakert6]

Of BoosterHandle but I think that's what you meant.

Sorry, but I didn't mean that. I really meant FastConfigHandle. Quoting you on #6021

This way, when processing on multiple threads, each thread can make its call to LGBM_BoosterPredictForMatSingleRowFastInit, then work without contention:

Here is the pseudo-code, would need to fill ... to make it compile. Also, it has memory/resource leakage.

#include <iostream>
#include <thread>
#include <vector>

#include "LightGBM/c_api.h"

// # of features
const int kFeatures = 13;
// # of threads which call predict
const int nworkers = 10;
// Shared booster
BoosterHandle booster_handle;

// Every thread exectues this function
void predict(ssize_t beg, ssize_t end) {
  // Each thread has its own FastConfigHandle
  FastConfigHandle config;
  int rc = LGBM_BoosterPredictForMatSingleRowFastInit(
      booster_handle, C_API_PREDICT_NORMAL, 0, 0, C_API_DTYPE_FLOAT64,
      kFeatures, "", &config);
  if (rc != 0) {
    abort();
  }
  for (ssize_t i = beg; i < end; i++) {
    int64_t len = 0;
    rc = LGBM_BoosterPredictForMatSingleRowFast(config, ...);
  }
}

int main(int argc, char* argv[]) {
  int num_iterations;
  int rc = LGBM_BoosterCreateFromModelfile(argv[1], &num_iterations,
                                           &booster_handle);
  if (rc != 0) {
    std::cout << "LGBM_BoosterCreateFromModelfile() returned " << rc << '\n';
    return 1;
  }
  std::vector<std::thread> workers(nworkers);
  for (ssize_t i = 0; i < nworkers; i++) {
    if (i != nworkers - 1) {
      workers[i] = std::thread(predict, ...);
    }
  }
  for (std::thread& t : workers) {
    t.join();
  }
  return 0;
}

If each thread creates its own Booster then memory usage for a single model would be x N, where N is the number of threads. So I want to share the booster with all threads and during prediction, I don't want to acquire the write lock, (that's why I am using your fork).

Let me know if my understanding of API usage is correct or not. I want to use your fork(because I need scalable predict/inference performance)

[Ten0]

Let me know if my understanding of API usage is correct or not

Your understanding is correct, it seems to be just a matter of definitions that caused our misunderstanding. (Copy, handle, globals...)

I want to use your fork(because I need scalable predict/inference performance)

Happy to help :)
If you happen to feel like helping to get this merged to not have to deal with the complications of using a fork I'd happily accept contributions to the PR with regards to the "please showcase usage in the C++ test suite" review comment - I'm not sure exactly when I'll have time to finalize this.

[Ten0]

TLDR; All comments resolved.

can you add a test to our C++ test suite showing multiple predictors using the fast API and sharing the booster?

That took a while to implement because a similar test didn't already exist (the C api was only tested through the python wrapper and the other implementations binding to it) but it's finally done! (1fcbc3f)

I've also added a workaround for #6142. The state of this issue is that, with the workaround, users would not hit issues any more with this API than they would with the current existing APIs around both single row and matrix prediction.

AFAICT this is ready for merge.

Side note: we've been using this in production for a few months without any issue (with the workaround implemented in the wrapper)

[Ten0]

@guolinke Static analysis complains that the TODO tag (#6024 (comment)) is not assigned. Who should I assign it to?

[jameslamb]

TODO

We prefer not using TODO comments at all, and instead using the Issues backlog to track future work. You won't find many in this repo:

• https://github.com/search?q=repo%3Amicrosoft%2FLightGBM%20TODO&type=code
• https://github.com/search?q=repo%3Amicrosoft%2FLightGBM+FIXME&type=code

Could you please put the content of that comment into a new issue at https://github.com/microsoft/LightGBM/issues with as much detail as necessary for someone else to pick it up (and linking to this PR)?

[guolinke]

Thank you!

[jameslamb]

Sorry, I'd missed that @guolinke approved this back in January!

I've just updated it to latest master. Will merge this today assuming that CI passes.

[jameslamb]

Thanks very much!