Code review request

[GoogleCodeExporter]

Purpose of code changes on this branch:
Please see http://code.google.com/p/aparapi/issues/detail?id=71

When reviewing my code changes, please focus on:
Code structure (Java/C++), JTP implementation, refactoring/architecture ideas

After the review, I'll merge this branch into:
/trunk

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

[GoogleCodeExporter]

Ryan 

I want to thank you and Steven for taking a stab at tidying this up. As we move 
forward I think this will help us move Aparapi forward and potentially add 
other GPU enabled back ends such as the HSA foundation's HSA architecture. 
http://hsafoundation.com/

Let me sync with the branch 
http://code.google.com/p/aparapi/source/browse/#svn%2Fbranches%2Faparapi-issues-
71-44
and start taking a look.  I know there is a lot of code changed here so this 
may take a while. 

Original comment by frost.g...@gmail.com on 29 Oct 2012 at 7:20

[GoogleCodeExporter]

Thanks Gary

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

[GoogleCodeExporter]

Would you mind if I ask the global mailing list to take on this task as well? 
We have made some additional C++ JNI changes, but are still tracking down the 
JTP bug.

Original comment by ryan.lam...@gmail.com on 19 Nov 2012 at 8:11

[GoogleCodeExporter]

The JTP bug has been resolved.

Original comment by ryan.lam...@gmail.com on 21 Nov 2012 at 2:54

[GoogleCodeExporter]

Hi Gary, just a ping on this issue, since it has been a month since the code 
review request and the trunk changes are really adding up.

Original comment by ryan.lam...@gmail.com on 26 Nov 2012 at 10:25

[GoogleCodeExporter]

Sorry Ryan.  I am on vacation (exhausted from todays Paragliding lessons ;) ) I 
will take a look at this when I get back (assuming I survive my lessons 
tomorrow and Wednesday!).

I do apologize.  

Gary

Original comment by frost.g...@gmail.com on 26 Nov 2012 at 10:56

[GoogleCodeExporter]

No problem Gary, please enjoy your vacation.
 On Nov 26, 2012 2:56 PM, <aparapi@googlecode.com> wrote:

Original comment by ryan.lam...@gmail.com on 27 Nov 2012 at 5:21

[GoogleCodeExporter]

Hi Gary, I wanted to know what the status on the code review is.  Right now 
it's blocking me on making progress on implementing multidimensional arrays.

Original comment by Steven.L...@gmail.com on 4 Dec 2012 at 7:27

[GoogleCodeExporter]

Steven, 

So I just synced with the latest code and took a quick look.  Of course my 
first test was to compile.  Compilation failed for me on Linux.  My guess is 
this will compile on Windows because my errors are due to file name case 
inconsistencies.   For example file clHelper.h is included as CLHelper.h and is 
failing.  My guess is this will succeed on Windows (is Mac OSX tolerant of 
this?). 

So my first suggestion is that we check code to ensure it is consistent (file 
name case with include case). 

Gary

Original comment by frost.g...@gmail.com on 4 Dec 2012 at 8:32

[GoogleCodeExporter]

I checked in some edits (r887) to allow header files to be located.  Now I have 
a bunch of warnings (string to char*) which may be OK. But can't get past these 
compile errors from gcc. 

src/cpp/invoke/opencljni.cpp: In instantiation of ‘void putPrimative(JNIEnv*, 
cl_kernel, jobject, jint) [with jT = int; cl_T = int; JNIEnv = JNIEnv_; 
cl_kernel = _cl_kernel*; jobject = _jobject*; jint = int]’:
src/cpp/invoke/opencljni.cpp:211:64:   required from here
src/cpp/invoke/opencljni.cpp:151:7: error: cannot pass objects of 
non-trivially-copyable type ‘std::string {aka class 
std::basic_string<char>}’ through ‘...’

and

src/cpp/invoke/OpenCLArgDescriptor.h:15:22: error: extra qualification 
‘OpenCLArgDescriptor::’ on member ‘getMemInstance’ [-fpermissive]
In file included from src/cpp/invoke/OpenCLArgDescriptor.h:4:0,
                 from src/cpp/invoke/OpenCLArgDescriptor.cpp:1:

Original comment by frost.g...@gmail.com on 4 Dec 2012 at 8:58

[GoogleCodeExporter]

Gary,

Apologies for the failures on Linux. We have thus far only tested on Windows 7 
64-bit (VS 2010) and OS X 10.7. We will work on getting the branch code into a 
Linux environment and testing it there.

Were you able to build in Windows 7 or OS X?

A couple of quick questions:

- What Linux are you using?
- What GCC are you using?

We've seen a number of inconsistencies between GCC versions that have caused us 
grief.

Original comment by ryan.lam...@gmail.com on 4 Dec 2012 at 10:31

[GoogleCodeExporter]

No problem. I just happened to be on a Linux machine waiting for a
build to finish. So I tried compiling there.

I was using  Ubuntu 64 bit 12.10 I think

gcc version on this machine was 4.7.2

BTW I checked my header changes in, and I think the other two issues
above can be solved with an appropriate cast.  But didn't want to
screw things up :)

Yes I did get it to build and run on Windows and started looking
through the code. We seemed to have lost maybe 5% performance
somewhere which is odd... can you guys confirm this? I have seen it on
two machines.... One with APU and one with GPU.

I will look deeper at the code.

I will re-iterate my concern about using STL.  Again, I will note that
OpenJDK JVM does not use it and I always worry about
portability/consistency. I want to try to re-factor some of this code
so it can be used inside the JVM (Sumatra) and I am a little reticent
to introduce dependencies which would not be compatible with the
OpenJDK code base.

Gary

Original comment by frost.g...@gmail.com on 4 Dec 2012 at 11:05

[GoogleCodeExporter]

It is interesting over the years hearing people's concerns about STL not being 
cross-platform. That may have been true prior to the late-1990's, but I cannot 
think of a legitimate reason today why a C++ developer would not use the STL. I 
think it is simply a historical mindset, exactly how "Java is slow" is 
widespread because the last time people used it was 1999. I'm guessing OpenJDK 
does not use STL because the code base was started in the early 1990's. What we 
do see though is people avoiding STL and then writing all of their own 
re-implementations of STL, which has all kinds of potential drawbacks. I think 
this is an important discussion, but definitely one which should have the 
OpenJDK folks weigh-in(if that is possible)...maybe there are actual technical 
reasons for not using STL in OpenJDK, maybe not?

Anyway, I'm very interested in the performance degradation. Do you have a 
specific test with timing that you would like us to verify?

Original comment by ryan.lam...@gmail.com on 5 Dec 2012 at 12:38

[GoogleCodeExporter]

Of course, we could always use the BoostLibraries instead :)

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

[GoogleCodeExporter]

:)

I was trying to track down a reason why OpenJDK seems to steer clear of STL.  I 
saw a few comments regarding not being able to control allocations... but I 
really can't say that there is a concrete reason.   I am just aware that it is 
not used.  Lots of home build collections, maps and sets..

Original comment by frost.g...@gmail.com on 5 Dec 2012 at 1:43

[GoogleCodeExporter]

Yes, unless I am mistaken, it is my understanding that since ~2003 pretty much 
all of the major compilers have standardized on the C++ Standard Library and 
Standard Library Template.

Original comment by ryan.lam...@gmail.com on 5 Dec 2012 at 2:31

[GoogleCodeExporter]

Ok, I went through and tried compiling the c++ code on linux.  I fixed the 
errors, so it compiles now, and I eliminated several of the warnings.  It turns 
out g++ takes const more seriously than visual studio does.  The only warnings 
I have now are related to printf statements getting arguments of different 
sizes than it expects.

Original comment by Steven.L...@gmail.com on 5 Dec 2012 at 6:46

[GoogleCodeExporter]

Thanks Steven. 

I will dive back in.  

Gary

Original comment by frost.g...@gmail.com on 5 Dec 2012 at 7:19

[GoogleCodeExporter]

Steven

I have reviewed the cpp code.  Still need to look at the java.  I like the 
cleanup that you have done, and want to thank you for it. 

I only have one request. Can we be consistent with file name case.  I note we 
have some  headers CLxxxx.h and others clxxxx.h.  I have built a CDT project 
for building/navigating this new structure from within Eclipse CDT. Let me know 
if you would find this useful, I will check the profile files into svn. 

I hope to get around to the Java changes soon.  I do apologize for my 
tardiness.  Lots of spinning plates at the moment. 

One more question, what is the strategy for the merge.  I guess we need to diff 
the current trunk with the version you copied for this branch and use this 
subset to update your branch (we have some small patches since the fork, I am 
sure). 

Gary 

Original comment by frost.g...@gmail.com on 6 Dec 2012 at 9:43

[GoogleCodeExporter]

Gary,

Apologies for the filename casing inconsistencies. I will speak with Steven 
about getting those corrected. Please feel free to refactor as necessary, we 
are hoping to full two-way development efforts in this branch.

Using Eclipse CDT would be highly desirable for me, at least and I would 
welcome your check-ins of that configuration for Aparapi. It will be 
interesting seeing CDT working cross-platform.

As for Trunk merging, what I imagine we'll have to do is the following:

- Take inventory of all changes to the Trunk since this branch was cut
- Merge in all Trunk changes to the branch
- Merge Branch into Trunk...which may just be a complete overwrite if Branch 
has all Trunk changes applied

I'm still very interested in tracking down the performance issues you are 
seeing. Are they in both JTP and GPU mode? Are they pre or post OpenCL 
generation? Do you have a specific test you were using that we can also use to 
track this together?

Original comment by ryan.lam...@gmail.com on 8 Dec 2012 at 1:41

[GoogleCodeExporter]

Gary,

That's a good point.  File name conventions in c++ are iffy at best.  I changed 
the file names to reflect the conventions in the java code, so now they're 
consistent across the project.

Steven

Original comment by Steven.L...@gmail.com on 10 Dec 2012 at 7:15

[GoogleCodeExporter]

Gary,

All C++ files and classes have been renamed to be consistent with Java.

Original comment by ryan.lam...@gmail.com on 11 Dec 2012 at 5:44

[GoogleCodeExporter]

Thanks Guys.  Still have not got back into Java code review.  I hope to do so 
over the next day or so.

Original comment by frost.g...@gmail.com on 11 Dec 2012 at 6:19

[GoogleCodeExporter]

Gary,

I've been investigating the 5% decrease in performance you mentioned. I hope to 
write an actual timing test soon, but I've not yet decided how that should be 
conducted, any suggestions are welcome.

Here is what I've seen so far on two sample projects:

Mandel FPS:
Trunk is equal to Branch

Life FPS:
Trunk is 5%-8% slower than Branch

Original comment by ryan.lam...@gmail.com on 14 Dec 2012 at 11:30

[GoogleCodeExporter]

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

• Changed state: Done