taoensso / nippy

The fastest serialization library for Clojure
https://www.taoensso.com/nippy
Eclipse Public License 1.0
1.04k stars 60 forks source link

non sync version of byte-array iostreams (from Apache Harmony, also used in Cassandra) #10

Closed mpenet closed 11 years ago

mpenet commented 11 years ago

PR from mail discussion.

It makes freezing a bit faster for the sample data on the benchmark (~10%), but it could yield more I suspect, I didn't really dig deeper.

Feel free to throw it away.

ptaoussanis commented 11 years ago

Heh heh, still at it? ;-) This looks very promising!

I'm merging for now since I'd like to take a proper look on my end. A few questions:

ptaoussanis commented 11 years ago

License seems okay: http://www.eclipse.org/legal/eplfaq.php

The current list of licenses approved for use by third-party code redistributed by Eclipse projects is:
Apache Software License 1.1
Apache Software License 2.0
mpenet commented 11 years ago

I didn't want to give up on speeding up freezing, not yet :)

About the license it seems we can include these file without changing anything really: http://www.eclipse.org/legal/eplfaq.php

What licenses are acceptable for third-party code redistributed by Eclipse projects? Eclipse views license compatibility through the lens of enabling successful commercial adoption of Eclipse technology in software products and services. We wish to create a commercial ecosystem based on the redistribution of Eclipse software technologies in commercially licensed software products. Determining whether a license for third-party code is acceptable often requires the input and advice of Eclipse’s legal advisors. If you have any questions, please contact license@eclipse.org.

The current list of licenses approved for use by third-party code redistributed by Eclipse projects is: Apache Software License 1.1 Apache Software License 2.0 W3C Software License Common Public License Version 1.0 IBM Public License 1.0 Mozilla Public License Version 1.1 Common Development and Distribution License (CDDL) Version 1.0 GNU Free Documentation License Version 1.3 BSD MIT

About "-target" "1.6", it was just a copy paste from another project, it might not be necessary after all.

On disadvantages, I don't think there is any, it just removes synchronized on all methods, so it's no longer thread safe, but since we are never exposing the streams, and keep everything contained it should not cause any issue.

It seems we could gain even more depending on the platform/jdk, I couldn't try yesterday, but this does sound promissing.

mpenet commented 11 years ago

Relevant, related article: http://java-performance.info/java-io-bytearrayoutputstream/

This seems it could also give a little boost to carmine almost without any modifications, I don't know what kind of impact it would have there.

On Mon, Jun 17, 2013 at 8:50 AM, Peter Taoussanis notifications@github.comwrote:

License seems okay: http://www.eclipse.org/legal/eplfaq.php

The current list of licenses approved for use by third-party code redistributed by Eclipse projects is: Apache Software License 1.1 Apache Software License 2.0

— Reply to this email directly or view it on GitHubhttps://github.com/ptaoussanis/nippy/pull/10#issuecomment-19528690 .

Max Penet


tel: +41 79 697 04 36 jabber: max.penet@gmail.com twitter: http://twitter.com/mpenet


ptaoussanis commented 11 years ago

Okay, this is looking good- think I'm going to wrap it up as alpha8. I see you've stuck the io-stream stuff in the com.taoensso package: is there no original official source for this?

I see there's a org.apache.cassandra.io.util package, but there also seem to be a dozen others that are all doing the same thing. Is everyone just taking the standard JDK code and manually stripping the synchronization stuff?

mpenet commented 11 years ago

The files I included are from the cassandra repository. Everybody is doing that it seems yes, there is an issue in commonss-io to have it included there, but it's still unresolved.

ptaoussanis commented 11 years ago

Okay, alpha8 is up with your changes and updated benchmarks (the chart still says alpha6, that's a typo). Cheers!

ptaoussanis commented 11 years ago

BTW Have given you commit access to the Nippy repo (not sure if they send a notification for that).

mpenet commented 11 years ago

Thanks! If I have new findings I will likely use PRs anyway, I like the reviewing process :) But this could be handy.

(Yep they do send notifications for this)

mpenet commented 11 years ago

FYI I have extracted this code into https://github.com/mpenet/grease , as I will also use it in internal/private projects. I also added BufferedIOStream unsynchronized versions.

I might also steal repeatedly-into :)

ptaoussanis commented 11 years ago

Just a heads-up that I'm going to be reversing the switch to the fast IO streams for now. Seem to be experiencing some problems operating across the usual range of JVMs. I suspect this is behind the original -target, -source flag motivation.

For a library like Nippy I'm inclined to be conservative and for the moment I don't have the time to dig into this in detail. Anyway, the major speed bumps were a result of reduce-into and the increased use of macros, so I don't feel too bad about dropping the last 2-3% gain for the added simplicity for now.

Will keep an eye on your grease lib and we can always come back to this later since it's such an easy switch.

Cheers!

mpenet commented 11 years ago

Sure, stability is more important. I am curious, what kind of issues did you have? I am likely to bump into the same problems on my side.

ptaoussanis commented 11 years ago

Might be something very simple, haven't looked into it at all - currently focused on the upcoming v2 Carmine release.

Leiningen 2.1.2 on Java 1.6.0_27 OpenJDK 64-Bit Server VM
install
$ lein2 deps
Retrieving org/clojure/tools.macro/0.1.2/tools.macro-0.1.2.pom from central
Retrieving org/clojure/pom.contrib/0.0.25/pom.contrib-0.0.25.pom from central
Retrieving org/clojure/clojure/1.3.0/clojure-1.3.0.pom from central
Retrieving commons-pool/commons-pool/1.6/commons-pool-1.6.pom from central
Retrieving org/apache/commons/commons-parent/22/commons-parent-22.pom from central
Retrieving org/apache/apache/9/apache-9.pom from central
Retrieving commons-codec/commons-codec/1.6/commons-codec-1.6.pom from central
Retrieving org/clojure/data.json/0.2.1/data.json-0.2.1.pom from central
Retrieving expectations/expectations/1.4.43/expectations-1.4.43.pom from clojars
Retrieving erajure/erajure/0.0.3/erajure-0.0.3.pom from clojars
Retrieving org/mockito/mockito-all/1.8.0/mockito-all-1.8.0.pom from central
Retrieving junit/junit/4.8.1/junit-4.8.1.pom from central
Retrieving com/taoensso/timbre/2.1.2/timbre-2.1.2.pom from clojars
Retrieving org/clojure/tools.macro/0.1.1/tools.macro-0.1.1.pom from central
Retrieving org/clojure/pom.contrib/0.0.20/pom.contrib-0.0.20.pom from central
Retrieving org/clojure/clojure/1.3.0-alpha5/clojure-1.3.0-alpha5.pom from central
Retrieving clj-stacktrace/clj-stacktrace/0.2.5/clj-stacktrace-0.2.5.pom from clojars
Retrieving com/taoensso/nippy/2.0.0-alpha10/nippy-2.0.0-alpha10.pom from clojars
Retrieving org/iq80/snappy/snappy/0.3/snappy-0.3.pom from central
Retrieving ring/ring-core/1.1.0/ring-core-1.1.0.pom from clojars
Retrieving org/clojure/clojure/1.2.1/clojure-1.2.1.pom from central
Retrieving commons-io/commons-io/2.1/commons-io-2.1.pom from central
Retrieving commons-fileupload/commons-fileupload/1.2.1/commons-fileupload-1.2.1.pom from central
Retrieving org/apache/commons/commons-parent/7/commons-parent-7.pom from central
Retrieving org/apache/apache/4/apache-4.pom from central
Retrieving javax/servlet/servlet-api/2.5/servlet-api-2.5.pom from central
Retrieving clj-time/clj-time/0.3.7/clj-time-0.3.7.pom from clojars
Retrieving joda-time/joda-time/2.0/joda-time-2.0.pom from central
Retrieving org/clojure/clojure/1.4.0/clojure-1.4.0.jar from central
Retrieving commons-codec/commons-codec/1.6/commons-codec-1.6.jar from central
Retrieving commons-pool/commons-pool/1.6/commons-pool-1.6.jar from central
Retrieving org/clojure/data.json/0.2.1/data.json-0.2.1.jar from central
Retrieving org/clojure/tools.macro/0.1.2/tools.macro-0.1.2.jar from central
Retrieving org/mockito/mockito-all/1.8.0/mockito-all-1.8.0.jar from central
Retrieving junit/junit/4.8.1/junit-4.8.1.jar from central
Retrieving org/iq80/snappy/snappy/0.3/snappy-0.3.jar from central
Retrieving commons-io/commons-io/2.1/commons-io-2.1.jar from central
Retrieving commons-fileupload/commons-fileupload/1.2.1/commons-fileupload-1.2.1.jar from central
Retrieving javax/servlet/servlet-api/2.5/servlet-api-2.5.jar from central
Retrieving joda-time/joda-time/2.0/joda-time-2.0.jar from central
Retrieving expectations/expectations/1.4.43/expectations-1.4.43.jar from clojars
Retrieving erajure/erajure/0.0.3/erajure-0.0.3.jar from clojars
Retrieving com/taoensso/timbre/2.1.2/timbre-2.1.2.jar from clojars
Retrieving clj-stacktrace/clj-stacktrace/0.2.5/clj-stacktrace-0.2.5.jar from clojars
Retrieving com/taoensso/nippy/2.0.0-alpha10/nippy-2.0.0-alpha10.jar from clojars
Retrieving clj-time/clj-time/0.3.7/clj-time-0.3.7.jar from clojars
Retrieving ring/ring-core/1.1.0/ring-core-1.1.0.jar from clojars
$ lein2 test-all
Performing task 'do' with profile(s): 'test,1.4'
Exception in thread "main" java.lang.UnsupportedClassVersionError: com/taoensso/FastByteArrayOutputStream : Unsupported major.minor version 51.0
    at java.lang.ClassLoader.defineClass1(Native Method)
    at java.lang.ClassLoader.defineClass(ClassLoader.java:634)
    at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
    at java.net.URLClassLoader.defineClass(URLClassLoader.java:277)
    at java.net.URLClassLoader.access$000(URLClassLoader.java:73)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:212)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.net.URLClassLoader.findClass(URLClassLoader.java:205)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:321)
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:294)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:314)
mpenet commented 11 years ago

I will have a look at it. Thanks

mpenet commented 11 years ago

Apparently this error indicates a version missmatch between javac and java. javac 1.7 was used to compile the files (with a default target of 1.7), and then java 1.6 can't run them.

I restored the compile flag to force 1.6 target/source compilation in cc.qbits/grease "0.2.1". I will give it a try after work and let you know if that fixes the issue on carmine's side.