[Truffle] Complete, correct and optimised implementation of Array#pack #2855
Conversation
Conflicts: truffle/src/main/java/org/jruby/truffle/nodes/core/ArrayNodes.java
| sh 'ruby', 'spec/mspec/bin/mspec', command, '--config', 'spec/truffle/truffle.mspec', *args | ||
| def mspec(env, command, *args) | ||
| env_vars = {} | ||
| mspec_env env_vars, command, *args |
There was a problem hiding this comment.
env is not used in the definition here and this will ignore the first argument. I think you meant to leave the arguments unchanged.
There was a problem hiding this comment.
Fixed in 0399ce5
|
It's a bit hard to grasp with the large diff so I think I'd better have a look in Eclipse instead. |
| @@ -0,0 +1,79 @@ | |||
| /* | |||
There was a problem hiding this comment.
I would group these "read primitive" nodes in a single file if that makes sense so to avoid n tiny files and ease possible refactoring.
There was a problem hiding this comment.
The read nodes are a screen of code each - they're not that tiny. Do you mean the write nodes? They are pretty small.
There was a problem hiding this comment.
Indeed, my mistake, I meant integer write nodes. But it's also good to keep one per file for consistency then.
|
@chrisseaton Here is a test with some pack usage found in protobuf/oga if you want to give this a try: Runs fine in jruby/ruby but saw some differences with truffle-pack branch. |
| RubyArray array, | ||
| RubyString format, | ||
| @Cached("privatizeByteList(format)") ByteList cachedFormat, | ||
| @Cached("create(compileFormat(format))") DirectCallNode callPackNode) { |
There was a problem hiding this comment.
The DSL expands it into DirectCallNode.create(compileFormat(format)). There is a lookup order for where it finds methods in guards, and one place is static members of the parameters's class. It's not just copied and pasted into the DSL generated code - there is a specific lookup.
Conflicts: truffle/src/main/java/org/jruby/truffle/nodes/core/ModuleNodes.java
This is an implementation of the
Array#packmethod - it's likeprintffor binary data.Until this patch, we have been using JRuby's pack implementation, but that was never going to be complete or correct, as it makes calls on objects and that will use JRuby's classes not Truffles. It was also never going to be optimised as it requires converting objects to JRuby and then back to Truffle.
This implementation compiles pack expressions to Truffle nodes, and so makes them available for compilation. It also includes an algorithm to try to recover loops in very long expressions that we found being used in the wild - a kind of reverse unrolling. The nodes are generally fairly linear, so it's not quite like a normal AST, but I've also decomposed the operations, so a single formatting directive becomes separate read, write and convert nodes, which all specialise independently.
For the first time I'm using the new DSL to cache the compiled expressions.
Also important is being able to PE through pack expressions. Pack is often used to convert a single value to a binary representation. The result is that for example we can constant fold this entire expression
[14].pack('C').getbyte(0) * 2.One final point is that I've gone to more lengths than usual to check the implementation is correct. It passes all 1596 specs of course, but I can also run the specs with compilation turned on, which we don't usually do, and with array storage randomisation.
I'd like to also be able to test with specs run repeatedly and with a very low compilation threshold, but there's still a lot to be done to make that work.
Not for merge yet - we'll keep the
truffle-headbranch as just the API delta for now and merge this after the release of 0.7.@thomaswue @eregon @nirvdrum @bjfish