Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default constructor for Record? #10

Open
bsless opened this issue Jun 9, 2021 · 4 comments
Open

Default constructor for Record? #10

bsless opened this issue Jun 9, 2021 · 4 comments

Comments

@bsless
Copy link

bsless commented Jun 9, 2021

I got curious and tried emitting a record with the library.
The awesome part is it just worked. The "hard" part was it did not emit the default constructor emitted by javac, so I had to write one myself.
Looking at some bytecode, generating it seems straightforward:

(defn emit-record-constructor
  [fields]
  (conj
   (into
    [[:aload 0]
     [:invokespecial :super :init [:void]]]
    cat
    (map-indexed
     (fn [i {:keys [name type]}]
       [[:aload 0]
        [:aload (inc i)]
        [:putfield :this name type]])
     fields))
   [:return]))

Do you think it should be added by default when no init is provided with the same desc? (when the class's flags have :record)

@jgpc42
Copy link
Owner

jgpc42 commented Jun 10, 2021

I did take a look at records a bit the last time I bumped the ASM version. At that time, I wasn't exactly sure what utility emitting records from Clojure would have, other than just emulating Java/javac. Please correct me if I'm wrong, but as far as I can tell, there's no special synthesis/magic about records at the JVM level; they're essentially just syntactical sugar for immutable value classes coupled with some boilerplate methods.

It seems like this might be a worthy addition, and with a minor change to support primitives (:iload, double-width types, etc.), your constructor function seems good (thanks!). However, after again reading the javadoc and taking a cursory look at the bytecode, I think that if we did do this, a hypothetical user of this feature might additionally expect the field accessor methods, along with equals, toString, et al., to be auto-generated and work similarly, as well. So I'm not sure right now what is best.

Also, I'd be curious to know if you have a use case for emitting records that I may have missed?

Thanks.

@bsless
Copy link
Author

bsless commented Jun 10, 2021

First, you are correct regarding some of the methods I neglected. That was my oversight, I simply didn't read the bytecode fully, instead focusing on the constructor.
With regards to the meat of the question:

there's no special synthesis/magic about records at the JVM level

I think records are slightly different at the jvm level than regular classes. I wasn't able to find it with a cursory glance at the source code, but iirc them being immutable value classes makes things easier for the JIT compiler, but I'm not sure.
I wouldn't mind playing around with the code to emit the minimal bytecode required for a record. Seems like a fun way to get to know the library, but with regards to the use case, the motivating example was about 80% intellectual curiosity, and 20% wondering if records could play well with Clojure. A particular use case could be that Clojure collections make for terrible hash map keys. Nominal tuples which already implement hash and equality would let us use compound keys with significantly less overhead.

@jgpc42
Copy link
Owner

jgpc42 commented Jun 10, 2021

You are right about the JIT having an easier time with immutable classes, but until I hear something about records being additionally optimized or otherwise treated specially by the JVM more than just normal immutable value classes, I think I'll put first class support for records in the core on hold.

I wouldn't mind playing around with the code to emit the minimal bytecode required for a record.

Sounds like a useful feature, maybe you could call your implementation defrecord--? :)

Now that I think about it, if we eventually do support records, something like you've mentioned may really just belong in a different namespace like insn.record, or maybe another library altogether for higher-level constructs (kind of like how ASM provides convenience methods with GeneratorAdapter). Maybe if you make any progress on this, your work could be the initial start to a new companion library? (Although, I'd need to expose visitRecordComponent in the core to enable 100% compliance with the Java compilation.)

Anyway, thanks for opening this. We can leave it open for the time being for any additional discussion.

@bsless
Copy link
Author

bsless commented Jun 10, 2021

Another thing to keep in mind which will be different in terms of bytecode and JVM representation is primitive classes (so records are defrecord- while primitive records will be --)
https://openjdk.java.net/jeps/401
Still a candidate, but looming on the horizon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants