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

Extract Kryo interface #235

Closed
brianfromoregon opened this issue Aug 13, 2014 · 13 comments
Closed

Extract Kryo interface #235

brianfromoregon opened this issue Aug 13, 2014 · 13 comments

Comments

@brianfromoregon
Copy link

I have a number of extensions of Kryo in my code base, either to fix bugs or add checks. Right now I have to override a number of methods and it feels brittle. If you had a Kryo interface I could implement instead and then delegate to the real Kryo, I'd sleep better at night :)

@serverperformance
Copy link
Contributor

What's wrong with inheritance + factory? (I'm old school).

I also have my customized XKryo subclass with overriden behaviour in some methods.

:)

-----Original Message-----
From: Brian Harris notifications@github.com
Date: Wed, 13 Aug 2014 13:41:21
To: EsotericSoftware/kryokryo@noreply.github.com
Reply-To: EsotericSoftware/kryo reply@reply.github.com
Subject: [kryo] Extract Kryo interface (#235)

I have a number of extensions of Kryo in my code base, either to fix bugs or add checks. Right now I have to override a number of methods and it feels brittle. If you had a Kryo interface I could implement instead and then delegate to the real Kryo, I'd sleep better at night :)


Reply to this email directly or view it on GitHub:
#235

@brianfromoregon
Copy link
Author

It's something that only became clear to me after my continual use of inheritance at work was burning me more often than not in terms of making future refactoring of the base class difficult, because base and (potential) children were more tightly coupled than I realized. A final class, or else a class with mostly final/private methods is a pleasure to refactor. It cannot be extended via inheritance but can be extended via composition. Think about what change could be made to the internals of the Kryo class today without potentially breaking users who are extending/overriding. There are very few of them! With Kryo devs giving increased attention to backward compatibility lately, working toward having an interface as the extension point would untie their hands for future refactoring. And I carry less risk because it's less likely that a Kryo version upgrade down the road will break my XKryo. For more reading google "favor composition over inheritance". It's not a java thing specifically, tho bloch does have two items (see 16 and 17) in Effective Java which cover it.

@romix
Copy link
Collaborator

romix commented Sep 23, 2014

@NathanSweet @brianfromoregon The idea of using interfaces in Kryo was suggested more than once. The usual concern was, IIRC, that it could introduce a performance hit. Nate should correct me, if I'm wrong. But eventually it is not the case anymore with modern JVMs. I think the easiest way to prove it is to try it out. Basically, decouple the Kryo class into an interface and implementation. Make sure all tests still work. Run some performance benchmarks, e.g. https://github.com/RuedigerMoeller/serialization-benchmark and see if any measurable overheads are introduced by this re-factoring.

@brianfromoregon Would you like to conduct this experiment and report back the results? It should be pretty straight-forward, hopefully.

@NathanSweet
Copy link
Member

Performance is not an issue with interfaces. The issue is that same as when any additional layers are added, the overall complexity is higher. I generally don't use interfaces until it can be seen that the abstraction is useful because I dislike the clutter.

Back in the good old days, Kryo was extremely simple to follow, now we have all sorts of factories and implementations. 😄 There are reasons for this of course, but the cost is that it is more complex.

I doubt a Kryo interface really reduces the chance that a future version of Kryo breaks something. Any significant refactoring probably breaks something anyway.

@romix
Copy link
Collaborator

romix commented Sep 23, 2014

Nate, thanks for your explanation. A small remark from my side: After having looked at the implementation of many Java serialization libs (Avro, Hazelcast serialization layer, fast-serialization, protostuff-runtime and many others), I'd say that Kryo is still rather simple and small in comparison with most/all of them. But yes, it is getting more complex with a time...

@brianfromoregon
Copy link
Author

An example from today. I wish Output was an interface because I'm writing an impl which writes to direct memory with Unsafe, and some of the methods I must inheirit doesn't make sense for me like setOutputstream, setBuffer, setPosition. An Output interface wouldn't have such methods I suppose.

@NathanSweet
Copy link
Member

An Output interface would probably have those methods. It doesn't hide that it uses a buffer.

@romix
Copy link
Collaborator

romix commented Sep 26, 2014

BTW, you know that you can write using Unsafe IO streams into direct memory already? Just use this constructor:
public UnsafeMemoryOutput (long address, int maxBufferSize), where address is the address in the off-heap memory.

Why do you need to implement an alternative solution?

@brianfromoregon
Copy link
Author

Hot dog! Thanks for the pointer I'll start with this!

A quick glance shows two things I might need to adjust

  • writeInt and others should do bounds checking with assert keyword so no penalty in prod
  • this memory is a region of a mmaped file, and I have that ByteBuffer being (privately) managed and written to elsewhere, so having UnsafeMemoryOutput create and write to its own buffer over the same address space might cause a problem? hmmm.. need to try it.

@romix
Copy link
Collaborator

romix commented Sep 27, 2014

Let me know if it worked for you. Also if you have any improvement proposals for those UnsafeMemoryInput/Output APIs, please let me know. I'm interested in making it even better.

@romix
Copy link
Collaborator

romix commented Nov 26, 2014

@brianfromoregon BTW, how did it work for you? Have you seen any improvements using Unsafe IO streams?

@brianfromoregon
Copy link
Author

Hi, did not adopt the unsafe streams because of issue discovered with read ops performing writes. May revisit again later.

@magro
Copy link
Collaborator

magro commented Feb 20, 2016

Closing, reopen if still relevant.

@magro magro closed this as completed Feb 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants