Re: [JGIT PATCH] 1/2: Externalizable items
- From
Shawn O. Pearce <spearce@spearce.org>
- Date
- Feb 16, 2009, 18:16 UTC
- Message-ID
- <20090216181659.GF18525@spearce.org>
- In-Reply-To
- <320075ff0902161009s1454e1feu5b3543f898112406@mail.gmail.com>
Nigel Magnay <nigel.magnay@gmail.com> wrote:
Show 12 quoted lines
> > Yikes. Do we really need a public no-arg constructor for > > Externalizable? If we do, maybe we should use Serializable instead > > so we can hide this constructor. I don't like the idea of people > > creating ObjectId.zeroId() by new ObjectId(). That's not a pattern > > we should encourage. > > > > Yes, you have to have a public no-args constructor for Externalizable. > > I agree, it's hideous. But I thought that was known as you explicitly > asked for Externalizable rather than Serializable with readObject / > writeObject... :-/
I forgot/didn't remember that Externalizable has this hideous requirement. I'd rather not introduce a public no-arg constructor just for Java serialization. So yea, if you needed to add a constructor than we should instead use Serializable and define an explicit serialVersionUID. Sorry.
> More than happy to re-roll with Serializable instead - do you want > this for all 4? (RemoteConfig also gained a no-args constructor > because of Externalizable..)
Yea, at least for ObjectId and RemoteConfig.
Show 11 quoted lines
> > + Map<String, Collection<String>> map = new HashMap<String,
> > Collection<String>>();
> > + for (int i = 0; i < items; i++) {
> > + String key = in.readUTF();
> > + String value = in.readUTF();
> >Why not just serialize the Map in the stream?
>
> Sure - if you're happy with that representation - it's not " a map of
> keys/values as it appears in the config " though as it's a map to a
> list because of the multi-values that are available for things like
> URL and Fetch.Right.
The format of HashMap and ArrayList is pretty stable on the wire, so we can depend on them not changing on us. We might as well just serialize with those and save ourselves some code in the JGit library. I don't see a compelling reason here to use our own map format on the wire. Especially not if we are doing this conversion to a java.util.Map<String,java.util.List> just to dump to the wire.
On the other hand, you could modify the read and write code to do direct writing of string pairs, and direct reading of string pairs, that would save allocs in each direction and make RemoteConfig that much easier to write out. Of course to do that you can't use a count header, but instead would want a sential marker to break the reader out of its loop, like an empty key string.
-- Shawn.