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

[tmva] MLP codegen threadsafety #572

Merged
merged 3 commits into from
May 19, 2017
Merged

Conversation

pseyfert
Copy link
Contributor

as it turnes out the ...class.C files generated by the TMVA MLP are not thread safe (fWeights is a constant array of contant pointers to beginnings of double arrays, and the contents therein vary at runtime inside the GetMvaValue__ method)

So the quick hack here is to replace the class member of dynamically allocated arrays by fixed sized arrays in the function scope.

QUASICODE OLD

class mlp {
  private:
    double *fweights[3]
    mlp() {
      fweights[0] = new double[5];
      fweights[1] = new double[10];
      fweights[2] = new double[1];
    }
    ~mlp() {
      delete fweights[0];
      delete fweights[1];
      delete fweights[2];
    }
    getmvavalue( std::vector<double> input) const {
       fweights[0] = input;
       fweights[1]  = some_function(fweights[0]);
       fweights[2] = some_other_function(fweights[1]);
       return fweights[2][0];
    }

QUASICODE NEW

class mlp {
  private:
    mlp() {
    }
    ~mlp() {
    }
    getmvavalue( std::vector<double> input) const {

      double fweights0[5];
      double fweights1[10];
      double fweights2[1];
       fweights0 = input;
       fweights1  = some_function(fweights0);
       fweights2 = some_other_function(fweights1);
       return fweights2[0];
    }

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@pseyfert
Copy link
Contributor Author

NB: opened the PR to make the draft public and open the discussion thread. no need to let jenkins run haven't tested the output of the codegen yet.

@pseyfert pseyfert force-pushed the THREADSAFETY branch 3 times, most recently from 1db4db6 to aecb0d7 Compare May 13, 2017 15:38
@pseyfert
Copy link
Contributor Author

  • root builds
  • formatting applied
  • ran tutorials/tmva/TMVAClassification.C (produces .class.C files)
  • class.C files compile
  • checked consistency with master branch with rapidcheck link

from my side good to merge.

NB: in the rapidcheck repo you can also see how my change changes the generated code by comparing the class.C files.

@pseyfert pseyfert changed the title WIP [tmva] MLP codegen threadsafety [tmva] MLP codegen threadsafety May 13, 2017
@gganis
Copy link
Member

gganis commented May 15, 2017

@phsft-bot build!

@phsft-bot
Copy link
Collaborator

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

@phsft-bot
Copy link
Collaborator

Build failed on mac1012/native.
See console output.

Warnings:

  • include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:62:50: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<double>::fOnes' required here, but no definition is available [-Wundefined-var-template]
  • include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:144:4: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<double>::fPool' required here, but no definition is available [-Wundefined-var-template]
  • include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:62:50: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<float>::fOnes' required here, but no definition is available [-Wundefined-var-template]
  • include/TMVA/DNN/Architectures/Cpu/CpuMatrix.h:144:4: warning: instantiation of variable 'TMVA::DNN::TCpuMatrix<float>::fPool' required here, but no definition is available [-Wundefined-var-template]

pseyfert and others added 3 commits May 17, 2017 10:58
use array variables in GetMvaValue__ rather than dynamically allocated
arrays, pointed to by a member array of double pointers.
use array variables in GetMvaValue__ rather than dynamically allocated
arrays, pointed to by a member array of double pointers.
suggestion from code review by @GerhardRaven
@vgvassilev
Copy link
Member

@phsft-bot build!

@lmoneta lmoneta merged commit e2f6003 into root-project:master May 19, 2017
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.

5 participants