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 global Environment singleton for PMacc #166

Conversation

c-schumann-zih
Copy link
Contributor

Pull request to discuss how to implement a single global Environment singleton in libPMacc, related to this issue.

It seems so far that a splitting into hpp and tpp files is necessary for classes that use __eventsystem_macros which moved into the new Environment.hpp.
This has been tested for TaskKernel and Buffer which need to be included in EventSystem.hpp now (whichis not great). However, more problems remain:

/home/schuma/picongpu_src/src/libPMacc/include/cuSTL/algorithm/kernel/run-time/Foreach.hpp(103):
error: too few arguments for class template "PMacc::Environment"

Any ideas on how to solve this better would be greatly appreciated :)


#include "eventSystem/tasks/Factory.hpp"
#include "eventSystem/Manager.hpp"
#include "eventSystem/transactions/TransactionManager.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Please only include EventSystem.hpp and no other classes in this folder. There are a lot of include loops inside thus you run very fast in compile problems which are very hard to solve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very good note, this helped a lot!

@ghost ghost assigned psychocoderHPC Jan 30, 2014
@ax3l
Copy link
Member

ax3l commented Jan 30, 2014

Hi Conrad,
thank you for your first pull request! ✨

I am sorry the test-environment created some weird output, but you spotted the problem yourself already. Just as a hint: manual tests can be done as described in this video - this produces much faster results than waiting for the suite.

I assigned @psychocoderHPC to the pull request since he probably knows best about the issues you reported (and @f-schmitt-zih is reviewing it anyway I guess).

Actually the EventSystem makros should be refactored (-> reads "removed") as a preparation for #133 . We might remove this makros already in a separate pull request and replace them by some nicer objects? Would you like to do that?



namespace PMacc {
//using namespace gol;
Copy link
Member

Choose a reason for hiding this comment

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

please remove me :)

@psychocoderHPC
Copy link
Member

Thanks for your pull request!

I think there is no need to create .def files, because before picongpu and game of life compile fine without those definitions. I will look tomorrow to your branch.
I will test to extract only your environment class and fix the compile problems without adding .def files.

@ax3l
Copy link
Member

ax3l commented Jan 30, 2014

The .def file is only intended for the new Environment class. The purpose is as I described:

if you change for example the number of templates of this class at some point in the future. It reduces also the Copy&Paste portion of the code.

@psychocoderHPC That will not solve the compile problems, but cleans up the implementation in secondary singletons.

-modefied Environment.hpp
-removed most of the forward declarations for class Environment
-removed some .tpp files
@c-schumann-zih
Copy link
Contributor Author

I modefied the Files with your comments and removed most of the forward declarations for class Environment and the code compiled well.

[100%] Built target gameOfLife ;D

@ax3l
Copy link
Member

ax3l commented Jan 30, 2014

yeah, cool 👍

[100%] Built target gameOfLife

I really have to fix the error message - I am quite interested in it :D

Edit: CompileSuite Message: Ah, your branch is also outdated/away from the dev of the mainline. I guess @f-schmitt-zih will love ^ to show you how to rebase against the current dev afterwards ;) ^sarcAnote1.0

@psychocoderHPC
Copy link
Member

+1 for getting gol to compile

@ax3l
Copy link
Member

ax3l commented Feb 21, 2014

Hi @c-schumann-zih, are you still working on that branch?
Do you like to rebase that branch or would you rather like to open a new pull request and close this one for now?

@c-schumann-zih
Copy link
Contributor Author

Hi, i'm just back from a two-weeks trainingcouse. I think i will ask f-schmitt-zih how to move on.

@ax3l
Copy link
Member

ax3l commented Feb 24, 2014

Nice, welcome back!

@c-schumann-zih
Copy link
Contributor Author

Hi @psychocoderHPC,
I added some more Singletons to the Environment.hpp. They compile well until I try to integrate the ParticleFactory. After adding ParticleFactory to the Environment.hpp i get errors like 'identifier "__getTransactionEvent" is undefined" ' and I can't figure out the reason. Maybe you can give me a hint.

@psychocoderHPC
Copy link
Member

OK I will check it.

@ax3l ax3l mentioned this pull request Feb 28, 2014
- make PArticleFactory getInstance private
- use Environment<> to get instance of ParticleFactory
- sove all dependencies
- fix all example compile time errors
@psychocoderHPC
Copy link
Member

I pushed an update to your pull request branch (here).

Updates:

  • add ParticleFactory to Environment
  • solve all dependencies
  • fix all compile time erros

…dParticleFactory

add ParticleFactory to Environment
@psychocoderHPC
Copy link
Member

Mh why is this pull red after @c-schumann-zih merged my pull request. I have tested all. If somene can see the error please feel free to fix them :-)

@ax3l
Copy link
Member

ax3l commented Mar 3, 2014

Probably due to a merge conflict...
He might have still to rebase against dev?

@c-schumann-zih
Copy link
Contributor Author

Pull request is replaced by #246

psychocoderHPC pushed a commit to psychocoderHPC/picongpu that referenced this pull request Apr 16, 2020
123cf3c952 Merge commit 'a1601e35c146c09d339239f076e477680982c2f7' into spec-cupla-alpaka0.5.0dev
a1601e35c1 Squashed 'alpaka/' changes from c2d14d09ea..b52eefa232
047884171f Merge pull request ComputationalRadiationPhysics#167 from psychocoderHPC/fix-ambiguousNamespace
a63eeb52e1 Merge pull request ComputationalRadiationPhysics#166 from psychocoderHPC/topic-moveMathDir
8564d2dc4f fix ambiguous namespace `device`
a0302ff66c move `cupla/math` to `cupla/device/math`
d6310a037e Merge pull request ComputationalRadiationPhysics#165 from psychocoderHPC/topic-updateGithubLinks2
e7f9261bf7 update cupla github links
f4dba001d4 Merge pull request ComputationalRadiationPhysics#162 from psychocoderHPC/topic-cuplaAccMathFunctions
bbbc0b0266 `BlackScholes` use cupla math functions
efa8d61037 on device math functions
dbc0f649b2 Merge pull request ComputationalRadiationPhysics#160 from psychocoderHPC/topic-updateAlpakaTo0.5.0dev
13b737ba6c Squashed 'alpaka/' changes from ab0b8a460..c2d14d09e
b31fc4ea33 Merge commit '13b737ba6cd38400636d6a6a52dee81b21a7412c' into topic-updateAlpakaTo0.5.0dev
09d5daf1ff update travis tests
582f8ca10e update CMake requirements
59fba492fa include changes needed to use alpaka 0.5.0

git-subtree-dir: thirdParty/cupla
git-subtree-split: 123cf3c952f4b5c4d2200fb9f2bb09bb84019614
psychocoderHPC pushed a commit to psychocoderHPC/picongpu that referenced this pull request May 18, 2020
b72833cbf0 Merge pull request ComputationalRadiationPhysics#169 from psychocoderHPC/topic-moreMathFunctions
b9944dc139 math functions: `erf` and `pow`
047884171f Merge pull request ComputationalRadiationPhysics#167 from psychocoderHPC/fix-ambiguousNamespace
a63eeb52e1 Merge pull request ComputationalRadiationPhysics#166 from psychocoderHPC/topic-moveMathDir
8564d2dc4f fix ambiguous namespace `device`
a0302ff66c move `cupla/math` to `cupla/device/math`
d6310a037e Merge pull request ComputationalRadiationPhysics#165 from psychocoderHPC/topic-updateGithubLinks2
e7f9261bf7 update cupla github links
f4dba001d4 Merge pull request ComputationalRadiationPhysics#162 from psychocoderHPC/topic-cuplaAccMathFunctions
bbbc0b0266 `BlackScholes` use cupla math functions
efa8d61037 on device math functions
dbc0f649b2 Merge pull request ComputationalRadiationPhysics#160 from psychocoderHPC/topic-updateAlpakaTo0.5.0dev
b31fc4ea33 Merge commit '13b737ba6cd38400636d6a6a52dee81b21a7412c' into topic-updateAlpakaTo0.5.0dev
13b737ba6c Squashed 'alpaka/' changes from ab0b8a460..c2d14d09e
09d5daf1ff update travis tests
582f8ca10e update CMake requirements
59fba492fa include changes needed to use alpaka 0.5.0

git-subtree-dir: thirdParty/cupla
git-subtree-split: b72833cbf0844664b26bbf553289356cfa03c18a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: PMacc in PMacc refactoring code change to improve performance or to unify a concept but does not change public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants