tdeneau / aparapi

Automatically exported from code.google.com/p/aparapi
Other
1 stars 1 forks source link

Aparapi Source Tree Organization #71

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
While I'm working through http://code.google.com/p/aparapi/issues/detail?id=44 
and http://code.google.com/p/aparapi/issues/detail?id=37 I've been thinking 
that it would be nice if we decomposed/refactored the Aparapi source code into 
appropriate packages, instead of having all code in a single package.

Part of the rationale for this is because I plan to add a .util package and 
possible some code into the .tools package, but also because it would make 
sense to have .opencl, .cuda and even .hsa packages to name a few.

Original issue reported on code.google.com by ryan.lam...@gmail.com on 8 Oct 2012 at 6:49

GoogleCodeExporter commented 9 years ago
Ryan

I think this is a great idea.  At one time (many moons ago) there were separate 
packages for writers (OpenCL and BaseWriter), classtools 
(MethodModel/ClassModel/InstructionSet) and execution (KernelRunner etc) 
others.  

The problem that I encountered (which I guess we could solve by sealing the 
jar) is that I wanted KernelRunner to access Kernel which meant that they could 
not be package protected.  Then I found the same tension between InstructionSet 
and MethodModel and in the end I punted and pulled them all into the same 
package. 

Certainly I would like to see .opencl and .hsa packages going forward. Not so 
sure about the .cuda one though ;) 

Maybe we can list all the classes and nominate package names and then change 
the build so that the jar is sealed to stop people 'guesting' into 
com.amd.aparapi

Gary

Original comment by frost.g...@gmail.com on 8 Oct 2012 at 7:15

GoogleCodeExporter commented 9 years ago
Okay, so far I've tentatively created the following structure:

com.amd.aparapi.jni
  JNILoader.java (new)
  OpenCLJNI.java

com.amd.aparapi.opencl
  OpenCL.java
  OpenCLAdapter.java
  OpenCLArgDescriptor.java
  OpenCLDevice.java
  OpenCLKernel.java
  OpenCLMem.java
  OpenCLPlatform.java
  OpenCLProgram.java

com.amd.aparapi
  <All other class files>

Thoughts? Any other suggestions?

Original comment by ryan.lam...@gmail.com on 9 Oct 2012 at 12:49

GoogleCodeExporter commented 9 years ago
Additional questions:

- Can we move all native methods from KernelRunner (and any other class) into 
OpenCLJNI or even a base class of OpenCLJNI?

I was thinking it would be nice to have all JNI code in a single package, 
instead of spread throughout various class files.

Two reasons for this:

- Since I started refactoring where the Java files are located, I'm running 
into a number of painful JNI naming problems (usually resulting in 
UnsatisfiedLinkErrors)

- I think it would give us a nice, clean single abstraction point between Java 
interfaces and JNI code

Original comment by ryan.lam...@gmail.com on 9 Oct 2012 at 6:52

GoogleCodeExporter commented 9 years ago
Makes perfect sense. 

So are you building this in a Branch so I can sing along ;) 

Or tinkering offline?

Gary 

Original comment by frost.g...@gmail.com on 9 Oct 2012 at 7:02

GoogleCodeExporter commented 9 years ago
Technically both :)

I was planning to check everything into a branch, but I'm apparently struggling 
with JNI at the moment, so I didn't want to check-in a broken mess.

If you have time, I can definitely check this into a branch and we can work on 
it together. Just let me know.

Original comment by ryan.lam...@gmail.com on 9 Oct 2012 at 8:15

GoogleCodeExporter commented 9 years ago
One man's broken mess is another mans 'opportunity for refactoring' ;)

So I would prefer in a Branch, I also think that it will help us hold a 
discussions and collaborate. 

I just started a MockLambda branch for playing with the Java 8 pre-release 
lambda branch. It is a pain to merge (if the branch stays out there too long) 
but it certainly helps manage the impact. 

Gary

Original comment by frost.g...@gmail.com on 9 Oct 2012 at 8:28

GoogleCodeExporter commented 9 years ago
Sounds great.

I'm working through what I think are naming issues. For example, I think in 
jnihelper.h there is a "#define AparapiPackage" that is trying to help out, but 
only if everything is in one package? So I renamed it AparapiBasePackage and 
created a new #define called AparapiOpenCLPackage...but then I think I may be 
getting bit by "#define JNI_JAVA" in the same class. Hmm...I need to reflect on 
this a little longer I think :)

Original comment by ryan.lam...@gmail.com on 9 Oct 2012 at 8:36

GoogleCodeExporter commented 9 years ago
I've checked-in a branch to "branches/aparapi-issues-71-44"

The code currently compiles but does not execute correctly, throwing 
UnsatisfiedLinkErrors...clearly the JNI problem has not been resolved yet.

This is a first pass at what Steven and I think would make a nice packaging 
scheme. The scheme is incomplete and may be wrong in some instances. Ideally, 
we'd like to add appropriate namespaces to the C++ code as well.

The com.amd.aparapi.jni package and objects were intended to create a clean 
abstraction and "single point of entry" for all of the JNI native access. I'm 
not sure we're %100 there yet, since some of the objects in 
com.amd.aparapi.opencl are still required by JNI, but I think it's very close. 
The current implementation is definitely up for discussion, specifically 
whether it's better to use value objects as a proxy or something else like base 
classes that non-JNI objects extend to get access to JNI-related native methods 
and fields.

Original comment by ryan.lam...@gmail.com on 12 Oct 2012 at 12:41

GoogleCodeExporter commented 9 years ago
I look forward to checking this out in the morning and helping to fix the 
unsatisfied link errors.

Thanks ryan and steven.

Original comment by frost.g...@gmail.com on 12 Oct 2012 at 12:43

GoogleCodeExporter commented 9 years ago
Thanks Gary, we look forward to the discussion.

Original comment by ryan.lam...@gmail.com on 12 Oct 2012 at 12:47

GoogleCodeExporter commented 9 years ago
"Houston we have a problem"

Apparently, when I exported my work to check-in to Google Code, my SVN client 
deleted all of my work...so I lost everything from this week. Bummer.

I'll let you know when/if I can figure this out :(

Original comment by ryan.lam...@gmail.com on 12 Oct 2012 at 1:21

GoogleCodeExporter commented 9 years ago
Oh no.  That sucks.  :)

I will offer the following as solice. Everytime this has happened to me (and it 
has happened more than I would like to admit) the coding of the second version 
was much quicker, and usually much cleaner than the first.

Gary 

Original comment by frost.g...@gmail.com on 12 Oct 2012 at 6:18

GoogleCodeExporter commented 9 years ago
Thanks Gary.

The lesson learned here is that if you Export from one folder to another
(e.g. branch folder), SVN only copies files that have been 'added to source
control' and not new files that you haven't yet checked in. Then of course,
when you revert your working Trunk workspace, all new files are lost.

I'm not necessarily blaming SVN, but since moving to Windows I've been
struggling a little with TortoiseSVN and apparently need to obtain a copy
of Beyond Compare just to double-check these kinds of things in the future.
And I really miss Time Machine :(

Microsoft really needs to buy/steal Time Machine from Apple :)

Original comment by ryan.lam...@gmail.com on 12 Oct 2012 at 6:44

GoogleCodeExporter commented 9 years ago
Or of course invent a real 'time machine' and put all these source repository 
problems behind us. How hard can it be, just need a Delorean and a Flux 
Capacitor. ;)

I think one of Douglas Adams' characters was forced to invent time travel 
because it was easier than learning how to program his VCR...

Original comment by frost.g...@gmail.com on 12 Oct 2012 at 6:47

GoogleCodeExporter commented 9 years ago
Okay, so I think I was able to reconstruct the lost files. And as you noted, I 
believe this time things have ended up a little bit cleaner.

So, the main idea that I'd like to toss around is the use of proxy/facade 
classes vs. base classes for JNI access.

In particular:

Config --> ConfigJNI (proxy) --> JNI could be converted to ConfigJNI (base 
class) --> Config --> JNI where all native methods and JNI accessed fields 
would reside in ConfigJNI.

That's my ASCII art representation of a UML diagram :)

The big question is whether base classes would work in all cases? Are should we 
consider an appropriate pattern per use case?

Right now, we have the following classes:

ConfigJNI
KernelArgJNI
KernelRunnerJNI
OpenCLJNI
RangeJNI

Original comment by ryan.lam...@gmail.com on 13 Oct 2012 at 12:13

GoogleCodeExporter commented 9 years ago
The branch code is in a state where it needs a code review. I will work on 
requesting that right now.

Steve is working on crafting an email/issue post highlighting the changes he'd 
like to discuss regarding the C++ code. He's already checked in a preliminary 
refactoring, but has a lot more ideas.

A couple of issues left to deal with:

- The JTP mode appears to be returning incorrect results, causing a number of 
the applications to display strange behavior. GPU mode appears to be correct. 
I'm guessing it is a minor typo or two in KernelRunner.

- I am still not entirely happy with how/where annotations live

- Any ideas to decouple Kernel/KernelRunner? One thought I had was to have 
Aparapi instantiate a singleton instance of KernelRunner and have KernelRunner 
accept instances of Kernel for execution (in a queue). There are other, 
potentially better solutions, I'm sure.

Original comment by ryan.lam...@gmail.com on 29 Oct 2012 at 6:21

GoogleCodeExporter commented 9 years ago
Also, the JNI code now emits the following when -Dcom.amd.aparapi.executionMode 
is excluded from the command-line:

no config !
created config !
created JNIContext !

Is/was this the desired behavior?

Original comment by ryan.lam...@gmail.com on 29 Oct 2012 at 6:51

GoogleCodeExporter commented 9 years ago

Original comment by ryan.lam...@gmail.com on 20 Apr 2013 at 12:34