Feature/large primitive arrays

[jarrodmoldrich]

@akbertram This is the first draft of the implementation. It compiles and passes the current test suite, but I have yet to write tests for the new functionality, or even try it with my production code, but just wanted to get your feedback. Would be much appreciated if you could take a look 🙂

Related to this issue

[jarrodmoldrich]

@akbertram Tests have been added, and are passing locally. I'll now try and integrate the modified plugin with our build.

[jarrodmoldrich]

@akbertram Is there any easy way for me to install a development build of the maven plugin in my local maven repository along with all the requisite dependencies? I'd like to test these code changes against our problematic C library. Any help would be appreciated.

[akbertram]

Unfortunately the gcc-bridge-maven-plugin hasn't been updated for the migration to gradle, so it's not actually being built yet. There are a few options, depending on whether you're committed to using Maven as a build tool for your project. 1. Rescue the sources from tools/gcc-bridge/maven-plugin and build as a separate maven project. It's not really possible to build a maven plugin with gradle, though I hacked something together for Renjin's maven plugin. 2. Use the maven exec:java goal to run org.renjin.gcc.Build 3. Update your project to use gradle, and then include GCC-Bridge as part of the build script. See https://github.com/bedatadriven/renjin-libstdcxx/blob/master/build.gradle for an example In either of the cases above, you'll want to run ./gradlew publishToMavenLocal in your local copy of Renjin to make your build accessible to maven/gradle locally. Hope this helps! Alex

…

On Wed, Nov 13, 2019 at 9:54 AM Jarrod Moldrich ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tools/gcc-bridge/compiler/src/main/java/org/renjin/gcc/codegen/constructors/Constructors.java <#489 (comment)>: > + return new VArrayExpr((GimpleArrayType) expr.getType(), new VPtrExpr(array)); + }; + + /* Convenience functions */ + + private static ByteBuffer constantArrayToBuffer( + GimpleConstructor value, + Predicate<GimpleType> isElementType, + BufferConverter converter + ) { + boolean isArrayType = isArrayWithType(value.getType(), isElementType); + if (!isArrayType) { + return null; + } + List<GimpleConstructor.Element> elements = value.getElements(); + if (elements.size() < 4000) { // only stream large arrays Thanks @akbertram <https://github.com/akbertram> 🙂 I was reading the spec last night and came to the same understanding, and I've updated the comments in arrays.c to match. So far I've been using the debug logging in the test results, but that javap looks really useful. I'm not sure if you missed my question, but I was wondering if there's a way I can generate a local gcc-build maven plugin? Such that I can build my c library in the same manner as gcc-bridge-example with this branch code? Otherwise I'll try generate my own script with the generated jars. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#489?email_source=notifications&email_token=AADO5Q3TLZLTYHRY3JWST4DQTO6DPA5CNFSM4JLVIH52YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLLQEPI#discussion_r345633421>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADO5Q3AJJE4Q2FLXLIMHIDQTO6DPANCNFSM4JLVIH5Q> .

-- Alexander Bertram Technical Director *BeDataDriven BV* Web: http://bedatadriven.com Email: alex@bedatadriven.com Tel. Nederlands: +31(0)647205388 Skype: akbertram

[jarrodmoldrich]

Thank you, @akbertram , I have started down the path of option #1, and I'm manually building a pom.xml that's partially taken from the old version. I'm noticing there's a number of repackaged libraries that I don't have access to. Am I going down the right path? Is there some way I can get or generate these dependencies?

        guava:              "org.renjin:renjin-guava:28.0b",
        asm:                "org.renjin:renjin-asm:5.0.4b",
        soot:               'org.renjin:renjin-soot:2.9.198'

[jarrodmoldrich]

Sorry @akbertram , also learning Maven as I go. Added the nexus repository to the pom and now that appears to be working fine... until the next problem :)

[akbertram]

Well I'd say you're a pretty quick learner!!

[akbertram]

The constructor is not guaranteed to contain a GimpleConstructor.Element for each array element. Sometimes GCC will emit 'jagged' arrays with uninitialized elements, mostly from Fortran code. See here:

renjin/tools/gcc-bridge/compiler/src/main/java/org/renjin/gcc/codegen/array/ArrayTypeStrategy.java

Line 179 in 5eb75d9

int elementIndex = ((GimpleIntegerConstant) element.getField()).getValue().intValue();

[jarrodmoldrich]

I see. Would inserting 0 values for these values be acceptable?

[akbertram]

Yes that's correct. I've extracted the normalizeArrayElements() logic to GimpleConstructor so that it can be used outside of ArrayTypeStrategy.

[jarrodmoldrich]

No worries, judging by your latest commits it looks like you've got it covered :)

[akbertram]

I am doing some testing on R packages that I know suffer from the problem of large array initializers, it's looking good!

[jarrodmoldrich]

@akbertram I've noticed something with the generated code. I'm compiling into a JAR and then looking at the decompiled version in Android Studio/IntelliJ which in turn uses 'FernFlower', and I'm seeing that static arrays are doubled up in the output:

    public static Ptr c_X$3467 = MixedPtr.malloc(7045120);
    public static Ptr $SomeClass$c_X = MixedPtr.malloc(7045120);

These arrays are quite massive to begin with, and Matlab C code loves to pre-allocate them on the stack. This means whenever the .class file is loaded into memory, these arrays both allocated, when really only one is used. Do you have any idea why it would duplicate these variables?

It's causing lots of OOM errors, so it's a deal breaker for using Renjin in our project, but it's so close to what we need. If there's an easy solution to removing these redundant arrays, I'd be grateful to hear it :)

EDIT:

This is a small snippet that should elicit the problem

void someFunction()
{
  static double someStaticArray[123456];
}

[jarrodmoldrich]

@akbertram I've worked around the issue by isolating all the function-scoped static large arrays and putting them in their own class - this allows me to allocate in one go, and moving the static vars outside of functions seems to remove the redundant entry.

Another issue of concern is that the gcc-bridge runtime, as it stands, cannot be used with Android < API 26 (i.e., before 'Oreo') as the MethodHandle.invoke and invokeExact methods are not available. I had to explicitly remove them to use the library like this

Do you think it would be possible to have conditional compilation for an Android build of the library? Or perhaps to use a different mechanism that is more compatible?

It would be nice if in the end our builds can target a sanctioned downstream release - but at the moment it looks like we need to target this custom branch.

[akbertram]

Re: excessive memory allocation. IIRC, the extra global variable (suffixed with $NNNN) are actually emitted by GCC as part of its strategy for compiling C functions with static local variables. A static local variable is not allocated on the stack, which is why some gymnastics on GCC's part are required.

I think the real problem lies with how we initializes global arrays. In principle, arrays are value types, so assignment involves copying. We naively use the assignment machinery to initialize global variables, so this involves 1) allocating the global variables, 2) constructing an initial value (another array), and 3) copying the initial value array into the allocated global variable.

This is trivially solved by splitting up the code generation for assignment and initialization, at least for arrays. This has been on my list for awhile, so I will see if quickly fix this.

You might also want to look at why you are seeing MixedPtrs in the output. MixedPtrs are used as a last resort when Renjin either can't figure out what type of data will be stored in an array or structure, or if a mix of primitives and pointers are stored in an array/structure. Note that this process relies on some basic static analysis as you can't trust always trust the declared type of arrays/structs. If Renjin is falling back to MixedPtrs when primitive arrays should clearly be used, I'd be interested in taking a looking at the source code.

[akbertram]

@jarrodmoldrich If it's just a problem with the runtime classes, we could probably configure gradle to produce a gcc-runtime-android.jar that simply excludes classes that rely on MethodHandles. The PosixThreads class for example, you're unlikely to need for your project.

The GimpleCompiler also uses MethodHandles to compile code that has function pointers. That's a much harder problem to work around, but it's probably not relevant for matlab-generated code.

[jarrodmoldrich]

Re: MixedPtr

MixedPtr is a rare case, and since I've re-organised the code I can only see one instance of it. It is an array of structs:

   typedef double real_T;
   typedef struct {
      real_T re;
      real_T im;
   } creal_T;
   extern creal_T B1[220160];

Re: MethodHandle

The MethodHandle class itself is available on Android, but I think specifically invoke and invokeExact are not provided until Oreo. It only matters for the runtime, or if these methods are called by the generated Java code. So PosixThreads and FunPtrCallGenerator are the only two exceptional classes that I can see.

[jarrodmoldrich]

@akbertram Just FYI, the MethodHandle errors were caused because my submodules gradle file was missing:

     compileOptions {
          sourceCompatibility JavaVersion.VERSION_1_8
          targetCompatibility JavaVersion.VERSION_1_8
     }

It compiles fine now. The last few days I've been playing with the transpiled code, and observed a couple of things. Firstly, it's possible that the code isn't quite correct. The original code is quite complex and it's possible that it's exercising the transpiler in a way that's eliciting undiscovered buggy use-cases. The other thing is that the code is several times slower, from ~5 sec to ~20+, though some of that time might be due to JIT compilation on the first run.

We've abandoned the JVM line of development for now, so I'm not sure I'll get to the bottom of the correctness issue - I might have a look in my own time though. In terms of the performance issues, my naive question would be whether many of the function calls could be inlined? I can't see a mechanism for warming up untouched code in Android, beyond actually making a dummy function call - but this is what I'd planned to do if there wasn't a good fix for code bloat at transpilation time. I'd be curious to know if there are any simple solutions?

Beyond all that, what are the next steps to making this PR merge-able? 🙂

[mjkallen]

@akbertram can you confirm that all changes in this PR have been merged into master as part of #498?