<< Back to previous view

[CLJ-1408] Add transient keyword to cached toString() value in _str Created: 19/Apr/14  Updated: 12/Sep/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Minor
Reporter: Tomasz Nurkiewicz Assignee: Brenton Ashworth
Resolution: Unresolved Votes: 0
Labels: interop

Attachments: Text File CLJ-1408-2.patch     Text File CLJ-1408-3.patch     Text File CLJ-1408.patch    
Patch: Code and Test
Approval: Screened

 Description   

_str field in Keyword and Symbol classes lazily caches result of toString(). Because this field is not transient, serializing (using Java serialization) any keyword or symbol before and after calling toString() for the first time yields different results:

(import (java.io ByteArrayOutputStream ObjectOutputStream
                 ByteArrayInputStream  ObjectInputStream))

(defn- serialize [obj]
  (with-open [bos (ByteArrayOutputStream.)
              stream (ObjectOutputStream. bos)]
    (.writeObject stream obj)
    (-> bos .toByteArray seq)))

;; keyword example

(def k1 (serialize :k))
(println :k)
(def k2 (serialize :k))

(= k1 k2) ;;=> false 

;; symbol example

(def sym 'a)

(def s1 (serialize sym))
(println sym)
(def s2 (serialize sym))

(= s1 s2) ;;=> false

This issue came up when I was trying to use keywords as key in [Hazelcast](https://github.com/hazelcast/hazelcast) map. Hazelcast uses serialized keys in various scenarios, thus if I first put something to map under key :k and then print :k, I can no longer find such key.

Approach: Add transient keyword to _str field in Keyword and Symbol classes

Patch: CLJ-1408-3.patch

Screened by: Brenton Ashworth



 Comments   
Comment by Alex Miller [ 19/Apr/14 7:28 AM ]

Hi Tomasz, it would be good to fix this, can you sign the CLA?

Comment by Tomasz Nurkiewicz [ 20/Apr/14 7:26 AM ]

Thanks, I'll sign and send CLA ASAP.

Comment by Tomasz Nurkiewicz [ 08/May/14 4:10 PM ]

My contributor greement arrived, please merge this patch whenever you find suitable.

Comment by Alex Miller [ 08/May/14 10:16 PM ]

Hi Tomasz, I noticed you added the private keyword - please remove that and update the patch.

Comment by Tomasz Nurkiewicz [ 09/May/14 3:55 PM ]

Removed `private` keyword

Comment by Alex Miller [ 20/Jun/14 9:22 AM ]

On second thought, it looks like we have most of the infrastructure for serialization testing anyways, so would appreciate an updated patch with the example turned into a serialization test. Please see test/clojure/test_clojure/serialization.clj for a place to put this (using existing roundtrip function).

Comment by Andy Fingerhut [ 01/Aug/14 9:29 PM ]

Tomasz, in addition to Alex's previous comment, it appears that a commit made to Clojure master earlier today causes your patches to no longer apply cleanly. I haven't looked to see whether updating the patches would be easy, but likely it is.

Comment by Alex Miller [ 22/Aug/14 11:00 PM ]

Updated the patch for latest master and added the obvious test.





[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 22/Sep/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-2.diff     File clj-1400-3.diff     File clj-1400-4.diff    
Patch: Code and Test
Approval: Screened

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch: clj-1400-4.diff

Screened by: Alex Miller



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)
Comment by Andy Fingerhut [ 29/Aug/14 4:46 PM ]

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Scott Bale [ 31/Aug/14 3:53 PM ]

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Comment by Alex Miller [ 09/Sep/14 9:29 AM ]

Few comments to address:

  • Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
  • the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
  • In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.
Comment by Scott Bale [ 11/Sep/14 11:19 PM ]

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.

  • I used tabs in Compiler.java
  • After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
  • (Test is unchanged)
Comment by Scott Bale [ 11/Sep/14 11:22 PM ]

(properly named patch)

Comment by Alex Miller [ 11/Sep/14 11:37 PM ]

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Comment by Scott Bale [ 19/Sep/14 2:37 PM ]

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the source line and col params to CompilerException are not available, or at least not afaict.





[CLJ-1330] Class name clash between top-level functions and defn'ed ones Created: 22/Jan/14  Updated: 25/Sep/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Alex Miller
Resolution: Unresolved Votes: 8
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1093-v2.patch     Text File 0001-CLJ-1093-v3.patch     Text File 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch     Text File 0001-Fix-CLJ-1330-refactored.patch     File demo1.clj    
Patch: Code
Approval: Screened

 Description   

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Demonstration script: demo1.clj

Patch: 0001-CLJ-1093-v3.patch

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.
The patch contains an additional enhancement to include the name of the local binding in the class name.

Comparison between pre and post patch naming scheme (N denotes unique number):

code before after note
(defn a []) user$a user$a same
(fn []) user$evalN$fn__N user$evalN$fn__N same
(fn a []) user$evalN$a__N user$evaN$a__N same
(let [a (fn [])] a) user$evalN$a__N user$evalN$a__N same
(let [a (fn x [])] a) user$eval1N$x__N user$evalN$a_x_N IMPROVED - contains local binding name
(def a (fn [])) user$a user$a same
(def a (fn x [])) user$x user$a_x_N FIXED conflict with (defn x [])
(def ^{:foo (fn [])} a) user$fn__N user$fn__N same
(def ^{:foo (fn a [])} a) user$a user$a__N FIXED conflict with (defn a [])
(def a (fn [] (fn []))) user$a$fn__N user$a$fn__N same
(def a (fn [] (fn x []))) user$a$x__N user$a$x__N same

See also: This patch also fixes the issue reported in CLJ-1227.

Screened by: Alex Miller - I am not sure whether the local binding name enhancement is worth doing. It improves debugging of which anonymous class you're talking about but has the downsides of increasing class name (and file name) length.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 22/Jan/14 11:12 AM ]

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Comment by Nicola Mometto [ 22/Jan/14 11:35 AM ]

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Comment by Nicola Mometto [ 22/Jan/14 11:39 AM ]

Cleaned up patch

Comment by Alex Miller [ 22/Jan/14 12:43 PM ]

It looks like the patch changes indentation of some of the code - can you fix that?

Comment by Nicola Mometto [ 22/Jan/14 3:57 PM ]

Updated patch without whitespace changes

Comment by Alex Miller [ 22/Jan/14 4:15 PM ]

Thanks, that's helpful.

Comment by Alex Miller [ 24/Jan/14 10:03 AM ]

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Comment by Nicola Mometto [ 24/Jan/14 11:39 AM ]

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.

Comment by Stuart Halloway [ 27/Jun/14 2:17 PM ]

incomplete pending the answers to Alex Miller's questions in the comments

Comment by Nicola Mometto [ 27/Jun/14 3:20 PM ]

I believe I already answered his questions, I'll try to be a bit more explicit:
I tracked the relevant commit from Rich which added the dynamic naming behaviour https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28#diff-f17f860d14163523f1e1308ece478ddbL3081 which clearly shows that this bug was present since then so.

Regarding redefinitions or recursive functions, both of those operations never take in account the generated fn name so they are unaffected.

Comment by Alex Miller [ 12/Sep/14 4:32 PM ]

Summarizing some cases here from before/after the patch:

1) top-level fn (always has name)
	1.6 - namespace$name
	patch - namespace$name
2) non-top-level fn with name
	1.6 - namespace$name (collides with prior case)
	patch - namespace$topname__x__name  	<-- CHANGED
3) anonymous fn (no name)
	1.6 - namespace$name$fn__x
	patch - namespace$name$fn__x
4) top-level anonymous fn (no name, not at all useful :)
	1.6 - namespace$fn__x
	patch - namespace$fn__x

The key problem is that the first 2 cases produce the identical class name on 1.6. The patch alters the non-top-level named fn so there is no conflict.

Prior to the referenced old commit, I believe cases 1 and 2 would both produce namespace$name__x (where x is unique) so they would not collide. The change was made to prevent the top-level name from changing ("don't append numbers on top-level fn class names"). While the similar change was made on non-top-level fn names, I do not think it needed to be.

I've thought through (and tried) a bunch of the implications of this with the help of Nicola's comments above and I do not see an issue with any of the things I've considered. From a binary compatibility point of view with existing AOT code, old code compiled together should be self-consistent and continue to work. New compiled code will also be consistent. I can't think of a way that new code would expect to know the old name of a non-top-level function such that there could be an issue.

One question - why change the code such that the new class name is namespace$name$topname__x__name instead of namespace$name$topname_name__x (or something else?). And relatedly, while the diff is small, could we refactor a couple more lines to make the intent and cases clearer?

I am 90% ok with this patch but want a little thought into that question before I mark screened.

Comment by Nicola Mometto [ 12/Sep/14 4:47 PM ]

Alex, the attached patch munges into ns$topname__name__x, not into ns$topname__x__name.

Comment by Nicola Mometto [ 12/Sep/14 5:22 PM ]

The attached patch 0001-Fix-CLJ-1330refactored.patch contains the same fix from 0001-FixCLJ-1330-make-top-level-named-functions-classnam.patch but also refactors the code that deals with fn name munging

Comment by Alex Miller [ 12/Sep/14 6:22 PM ]

Hmmm.. I will double-check. That's not why I recall seeing when I did AOT.

Comment by Nicola Mometto [ 12/Sep/14 7:26 PM ]

New patch 0001-CLJ-1093-v2.patch improves the fn naming scheme a lot.
I've threw together a number of test cases that show the improvement + bug fixes:

user=> (fn [])
;; pre:
#<user$eval1$fn__2 user$eval1$fn__2@4e13aa4e>
;; post: (no change)
#<user$eval1$fn__3 user$eval1$fn__3@3c92218c>
user=> (fn a [])
;; pre:
#<user$eval5$a__6 user$eval5$a__6@6946a317>
;; post: (no change)
#<user$eval6$a__8 user$eval6$a__8@6f85c59c>
user=> (let [a (fn [])] a)
;; pre:
#<user$eval9$a__10 user$eval9$a__10@15fdf894>
;; post: (no change)
#<user$eval11$a__13 user$eval11$a__13@4d051922>
user=> (let [a (fn x [])] a)
;; pre: (only contains the name of the fn)
#<user$eval17$x__18 user$eval17$x__18@7f0cd67f>
;; post: (contains the name of the local aswell as the name of the fn
#<user$eval21$a__x__23 user$eval21$a__x__23@528ef256>
user=> (def a (fn [])) a
#'user/a
;; pre:
#<user$a user$a@33e1ccbc>
;; post: (no change)
#<user$a user$a@6bef63f9>
user=> (def a (fn x [])) a
#'user/a
;; pre: (BUG!)
#<user$x user$x@59a04a1b> 
;; post: (bug fixed)
#<user$a__x__28 user$a__x__28@5f0bebef>
user=> (def ^{:foo (fn [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre:
#<user$fn__23 user$fn__23@d9c21c6>
;; post: (no change)
#<user$fn__30 user$fn__30@4cf0f2eb>
user=> (def ^{:foo (fn a [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre: (BUG!)
#<user$a user$a@420dd874>
;; post: (bug fixed)
#<user$a__35 user$a__35@37ff95a9>
user=> (def a (fn [] (fn []))) (a)
#'user/a
;; pre:
#<user$a$fn__30 user$a$fn__30@6f57be76>
;; post: (no change)
#<user$a$fn__41 user$a$fn__41@fd34eac>
user=> (def a (fn [] (fn x []))) (a)
#'user/a
;; pre:
#<user$a$x__35 user$a$x__35@79930089>
;; post: (no change)
#<user$a$x__48 user$a$x__48@6fc334de>
user=> (let [x (fn [])] (def a (fn [] x))) a (a)
#'user/a
;; pre:
#<user$eval40$a__43 user$eval40$a__43@6db1694e>
#<user$eval40$x__41 user$eval40$x__41@20bd16bb>
;; post (no change)
#<user$eval54$a__58 user$eval54$a__58@7c721de>
#<user$eval54$x__56 user$eval54$x__56@43f7b41b>
user=> (let [x (fn a [])] (def a (fn [] x))) (a)
#'user/a
;; pre: (the local binding name doesn't appear in the class name)
#<user$eval48$a__49 user$eval48$a__49@75d6d1d4>
;; post: (the local binding name is included in the class name)
#<user$eval64$x__a__66 user$eval64$x__a__66@460d4>

As you can see, this last patch not only fixes the two bugs, but also improves fn naming in let contexts by preserving the name of the local binding in the class name, this I believe will be a great improvement in the understandability of stacktraces.

Comment by Alex Miller [ 25/Sep/14 7:00 AM ]

The patch should be changed to not create suffix if it's not going to be used. Please update the patch to inline that into each branch name = nm.name + "__" + RT.nextID();.

I am unsure whether the "enhancement" part of this patch goes too far. I think it does provide some improvements in debugging but those seem small to me. I am somewhat concerned about greatly increasing the name of the class for nested locals thus making it harder to read stack traces. There is a large limit to class name size of 16 bits (what you can put in the constant table) but class names also map to file names and there have historically been issues on some older Windows architectures with file size limits - we are increasing the risk of running into problems with this. Small risks. I am ok with passing this on to Rich though and he can decide whether to kick that part back or not.

Comment by Nicola Mometto [ 25/Sep/14 7:08 AM ]

0001-CLJ-1093-v3.patch is identical to 0001-CLJ-1093-v2.patch except it doesn't call RT.nextID() when not necessary, as per Alex's request

Alex, if this is ok please change the "Patch:" field in the description, I won't do that myself since this ticket is now screened





[CLJ-803] IAtom interface Created: 27/May/11  Updated: 26/Sep/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Pepijn de Vos Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: atom

Attachments: Text File 0001-atom-interface.patch     Text File iatom.patch    
Patch: Code
Approval: Screened

 Description   

Atom does not have an interface and is marked final. This patch extracts an IAtom interface and has Atom implement it.

Patch: 0001-atom-interface.patch

Screened by: Alex Miller

More info: Rich said "patch welcome for IAtom". See IRC logs: http://clojure-log.n01se.net/date/2010-12-29.html#10:04c



 Comments   
Comment by Stuart Halloway [ 27/May/11 2:33 PM ]

Please add a patch formatted by "git format-patch" so that attribution is included.

Comment by Pepijn de Vos [ 04/Jun/11 5:56 AM ]

I added the formatted patch a few days ago. Does 'no news is good news' apply here?

And, silly question, will it make it into 1.3? I can't figure out how to tell Jira to show me that.

Comment by Kevin Downey [ 04/Jul/11 9:06 PM ]

I fail to see the need for an IAtom, if you want something atom like for couchdb the interfaces are already there. Maybe I ICompareAndSwap. Atoms and couchdb are different so making them appear the same is a bad idea.

http://en.wikipedia.org/wiki/Fallacies_of_Distributed_Computing

http://clojure.org/state one of the distinctions between agents and actors raised in the section titled "Message Passing and Actors" is local vs. distributed and the same distinction can be made between couchdb (regardless of compare and swap) and atoms

Comment by Aaron Bedra [ 04/Jul/11 9:18 PM ]

This ticket has already been moved into approved backlog. It will be revisited again after the 1.3 release where we will take a closer look at things. For now, this will remain as is.

Comment by Aaron Craelius [ 10/Jul/14 12:15 PM ]

Any chance this patch could get implemented in an upcoming Clojure release. There is still continued interest, see this thread: https://groups.google.com/forum/#!topic/clojure-dev/y5QoMqd44Lc

One suggestion I would make is also removing the final marker from clojure.lang.Atom - I can see use cases where one would want to directly subclass Atom (to capture dependencies in reactive computations for instance).

Comment by Brandon Bloom [ 02/Aug/14 2:14 PM ]

I'd like to see an IAtom interface, but would prefer that `swap` not be part of it. Swapping can, and should, be defined in terms of `compareAndSet`. Seems like IAtom should only have `boolean compareAndSet(object oldval, object newval)` as well as `void reset(object newval)`.

Comment by Brandon Bloom [ 02/Aug/14 2:29 PM ]

Alternative patch that introduces IAtom and converts swap to be static.

Comment by Pepijn de Vos [ 03/Aug/14 2:59 AM ]

At the time I did the initial patch, I had the same idea to remove swap, but Rich said there where cases for having it, so it should stay in according to him.

Comment by Aaron Craelius [ 03/Aug/14 1:51 PM ]

One use case I can think of for overriding swap is if an IAtom is wrapping say a row of data stored in a database. Then comparing something like a version column (or transaction id in the case of datomic) is what should determine whether a swap is retried, not the actual value of the data. In this case then, compareAndSet would actually be a more complex operation than swap and it makes sense to define the two independently.

Comment by Aaron Craelius [ 03/Aug/14 1:56 PM ]

I should also mention my related issue: http://dev.clojure.org/jira/browse/CLJ-1470 which simply allows Atom (and also ARef) to be sub-classed. Both patches could ultimately work together to make the whole Atom/ARef infrastructure easier to extend.

Comment by Alex Miller [ 10/Sep/14 4:47 PM ]

This ticket needs some work before it can be screened:

  • description can lose the the Couch stuff
  • needs some mention of tradeoffs for the various swap alternatives
  • don't know what patch should be considered
Comment by Brandon Bloom [ 10/Sep/14 5:01 PM ]

I removed my patch because Pepijn/Rich is right: Swap should be part of the interface.





Generated at Tue Sep 30 19:24:16 CDT 2014 using JIRA 4.4#649-r158309.