|
The problem is that a 'loop form gets converted into an anonymous fn that gets called immediately, when the loop is in a expression context (eg. its return value is needed, but not as the return value of a method/fn).
so
gets converted into
see the code in the compiler:
http://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L5572
this conversion already bites you if you have mutable fields in a deftype and want to 'set! them in a loop
http://dev.clojure.org/jira/browse/CLJ-274
|
|
loops in expression context are lifted into fns because else Hotspot doesn't optimize them.
This causes several problems:
- type inference doesn't propagate outside of the loop[1]
- the return value is never a primitive
- mutable fields are inaccessible
- surprise allocation of one closure objects each time the loop is entered.
Adressing all those problems isn't easy.
One can compute the type of the loop and emit a type hint but it works only with reference types. To make it works with primitive, primitie fns aren't enough since they return only long/double: you have to add explicit casts.
So solving the first two points can be done in a rather lccal way.
The two other points require more impacting changes, the goal would be to emit a method rather than a fn. So it means at the very least changing ObjExpr and adding a new subclassof ObjMethod.
[1] beware of CLJ-1111 when testing.
|
|
I don't think this is going to make it into 1.6, so removing the 1.6 tag.
|
|
an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn
|
|
I have attached a 3 part patch as hoistedmethod-pass-1.diff
3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class
9c39cac1 uses HoistedMethod to compile loops not in the return context
901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)
|
|
with hoistedmethod-pass-1.diff the example code generates bytecode like this
the body of the method __hoisted1677 is the inner loop
for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version
|
|
hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr
with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass
|
|
Thanks for the work on this!
|
|
I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do
|
|
hoistedmethod-pass-3.diff
49782161 * add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 * hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required
all contribs whose tests pass with master pass with this patch.
the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot
|
|
Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class
Test case:
user=> (fn [] (try 1 (finally)) 2)
VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack user/eval1 (NO_SOURCE_FILE:1)
|
|
bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too
|
|
hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code
|
|
hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff
|
|
 Kevin Downey FWIW the patch no longer applies
|
|
A naive attempt to bring the patch up to date results in
compile-clojure:
[java] Exception in thread "main" java.lang.ExceptionInInitializerError
[java] at clojure.lang.Compile.<clinit>(Compile.java:29)
[java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7154)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7154)
[java] at clojure.lang.Compiler.access$300(Compiler.java:38)
[java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7154)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7112)
[java] at clojure.lang.Compiler.eval(Compiler.java:7416)
[java] at clojure.lang.Compiler.load(Compiler.java:7859)
[java] at clojure.lang.RT.loadResourceScript(RT.java:372)
[java] at clojure.lang.RT.loadResourceScript(RT.java:363)
[java] at clojure.lang.RT.load(RT.java:453)
[java] at clojure.lang.RT.load(RT.java:419)
[java] at clojure.lang.RT.doInit(RT.java:461)
[java] at clojure.lang.RT.<clinit>(RT.java:331)
[java] ... 1 more
[java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
[java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351)
[java] ... 17 more
|
|
the pass 6 patch builds cleanly on 1d5237f9d7db0bc5f6e929330108d016ac7bf76c(HEAD of master as of this moment) and runs clojure's tests. I have not verified it against other projects as I did with the previous patches (I don't remember how I did that)
|
|
I get the same error I shared before applying the new patch to 1d5237f
|
|
I get some whitespace warnings with hoistedmethod-pass-6.diff but the patch applies. On a compile I get:
compile-clojure:
[java] Exception in thread "main" java.lang.ExceptionInInitializerError
[java] at clojure.lang.Compile.<clinit>(Compile.java:29)
[java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7156)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7343)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7156)
[java] at clojure.lang.Compiler.access$300(Compiler.java:38)
[java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:588)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7355)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7156)
[java] at clojure.lang.Compiler.analyze(Compiler.java:7114)
[java] at clojure.lang.Compiler.eval(Compiler.java:7418)
[java] at clojure.lang.Compiler.load(Compiler.java:7861)
[java] at clojure.lang.RT.loadResourceScript(RT.java:372)
[java] at clojure.lang.RT.loadResourceScript(RT.java:363)
[java] at clojure.lang.RT.load(RT.java:453)
[java] at clojure.lang.RT.load(RT.java:419)
[java] at clojure.lang.RT.doInit(RT.java:461)
[java] at clojure.lang.RT.<clinit>(RT.java:331)
[java] ... 1 more
[java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
[java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
[java] ... 17 more
|
|
Looks like direct linking interacts with the diffs in this patch in non trivial ways.
|
|
I must have screwed up running the tests some how, I definitely get the same error now
|
|
after you get past the cast issuing (just adding some conditional logic there)
it looks like HoistedMethodInvocationExpr needs to be made aware of if it is emitting in an instance method or a static method, and do the right thing with regard to this pointers and argument offsets. this will likely require HoistedMethod growing the ability to be a static method (and maybe preferring static methods when possible).
if you cause HoistedMethod to set usesThis to true on methods that use it, then everything appears hunky-dory (if I ran the tests correctly), but this largely negates the new direct linking stuff, which is not good.
|
|
hoistedmethod-pass-7.diff adds a single commit to hoistedmethod-pass-5.diff
the single commit changes hoisted methods to always be static methods, and adjusts the arguments in the invocation of the hoisted method based on if the containing function is a static/direct function or not.
again I haven't done the extended testing with this patch.
here is an example of what the hoisted methods look like
|
|
there is still an issue with patch 7, (defn f [^long y] (let [x (try (+ 1 0) (catch Throwable t y))] x)) causes a verifier error
|
|
Patch 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch is patch hoistedmethod-pass-7 with the following changes:
- fixes the bug mentioned in the last comment by Kevin Downey
- removes unnecessary whitespace and indentation changes
- conforms indentation with the surrounding lines
- squashes the commits into one as preferred by Rich
Authorship is maintained for Kevin Downey
|
|
maybe we need a "Bronsa's Guide to Submitting Patches" to supplement http://dev.clojure.org/display/community/Developing+Patches, I had no idea a single commit was preferred, but that makes sense given the format, although I just noticed the bit on deleting old patches to avoid confusion. Is there a preferred format for patch names too?
If you have recommendations beyond patch formatting into the code itself I am all ears.
|
|
Kevin, having a single commit per patch is something that I've seen Rich and Alex ask for in a bunch of tickets, as I guess it makes it easier to evaluate the overall diff (even though it sacrifices granularity of description).
No idea for a preferred patch name, I find it hard to imagine it would matter – I just use whatever git format patch outputs.
One thing I personally prefer is to add the ticket name at the beginning of the commit message, it makes it easier to understand changes when using e.g. git blame
|
|
Just now I added a suggestion to http://dev.clojure.org/display/community/Developing+Patches that one read their patches before attaching them, and remove any spurious white space changes. Also to consider submitting patches with a single commit, rather than ones broken up into multiple commits, as reviewers tend to prefer those.
Alex Miller recently edited that page with the note about putting the ticket id first in the commit comment.
Only preference on patch file names is the one on that page – that they end with '.patch' or '.diff', because Rich's preferred editor for reading them recognizes those suffixes and displays the file in a patch-specific mode, I would guess.
|
|
0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch is the same as 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch but it changes some indentation to avoid mixing tabs and spaces
|
|
I have a verify error on the latest patch, will attempt to provide a small test case:
Exception in thread "main" java.lang.VerifyError: (class: com/climate/scouting/homestead_test$fn__17125, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object Expecting to find integer on stack, compiling:(homestead_test.clj:43:3)
|
|
https://github.com/MichaelBlume/hoist-fail
I'd rather remove the dependency on compojure-api but I'm having trouble reproducing without it
|
|
Ok, less stupid test case:
(let [^boolean foo true]
(do
(try foo
(catch Throwable t))
nil))
|
|
...I wonder how hard it would be to write a generative test which produced small snippets of valid Clojure code, compiled them, and checked for errors. I bet we could've caught this.
|
|
Michael Blume I'm not sure this should be considered a compiler bug.
Contrary to numeric literals, boolean literals are never emitted unboxed and type hints are not casts so is lying to the compiler telling it a is an unboxed boolean when infact it is a boxed Boolean.
I'd say we wait for what Alex Miller or Rich Hickey think of this before considering Kevin Downey's current patch bugged, I personally (given the current compiler implementation) would consider this an user-code bug, currently ignored, that this patch merely exposes. IOW an instance of currently working code that is not however valid code
(I rest my case that if we had some better spec on what type-hints are supposed to be valid and some better compile-time validation on them, issues like this would not arise)
|
|
If this is considered a bug, the fix is trivial btw
@@ -5888,7 +5888,7 @@ public static class LocalBinding{
public boolean hasJavaClass() {
if(init != null && init.hasJavaClass()
- && Util.isPrimitive(init.getJavaClass())
+ && (Util.isPrimitive(init.getJavaClass()) || Util.isPrimitive(tag))
&& !(init instanceof MaybePrimitiveExpr))
return false;
return tag != null
|
|
I imagine ^boolean type hints (that don't do anything and are ignored) are going to become very common in clojure code, given everyone's keen interest in code sharing between clojure and clojurescript, and iirc clojurescript actually using ^boolean
|
|
Last patch doesn't compile anymore since it hits the bug reported in CLJ-1809
|
|
Moving to incomplete for now since it seems to be blocked on the other ticket
|
|
0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch makes the change that Nicola suggested (with an extra null check). Michael's test case with the ^boolean type hint compiles now
|
|
Maybe this is out of scope for this ticket since it's just existing code that you're moving around, but I've always been confused by
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ISTORE), finallyLocal);
finallyExpr.emit(C.STATEMENT, objx, gen);
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ILOAD), finallyLocal);
If we emit finallyExpr as C.STATEMENT, it should have no impact on the stack, and the exception should just be there without us needing to store and load it, right?
|
|
Also the latest patch seems to add a patch file to the root directory
|
|
New failing code
(defn foo [req]
(let [^boolean b (get req :is-baz)]
(do
(try
:bar)
:foo)))
|
|
the latest patch applied to master is causing some test failures for data.xml
|
|
Michael Blume that's still an invalid type hint. Clojure is inconsistent all over the place with enforcing valid type hints, so even though I think we should handle the VerifyError explicitly, I don't think we need to care about making that code work.
WRT ^boolean type hints going to be common because clojurescript uses them – I don't think we should accept invalid code for the sake of interoparability. See CLJ-1883 for an instance of such a wrong type hint being used in Om, the solution was to fix Om, not to make the clojure compiler ignore yet another wrong type hint.
|
|
I've just got around to trying to dig in to the data.xml failures I mentioned above.
Both those tests are related to head holding on a lazy seq. Both tests reliably fail with 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch
So I suspect the hoisted method isn't properly clearing locals.
|
|
so I don't forget, I realized the issue with data.xml is because the 'is' macro in the test expands in to a try/catch in an expression context, which is hoisted, and the hoist causes the whole environment not to be cleared until the hoisted method returns, which is obviously not correct.
|
Generated at Tue Feb 19 08:51:49 CST 2019 using JIRA 4.4#649-r158309.
0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch
hoistedmethod-pass-1.diff