[Json-rpc-java] registerSerializer

[Michael Clark]

Raziel Alvarez wrote:
> I'm facing what seems to me as a big performance issue in the
> JSONSerializer
> code.
>
> My application uses a considerable amount of custom serializers. Some of
> them need to be updated with some contextual information in order to
> perform
> a proper serialization. Everytime a serialization is needed, and that
> contextual information changes, we create new serializer instances and
> re-register them (in the existing JSONSerializer instance) using the
> registerSerializer(Serializer s) method in JSONSerializer class.

perhaps an unregsiterSerializer(Serializer s) would help? (we have this
method for most of the items that can be registered against the bridge).

> The problem is that this method verifies if a serializer already exists
> checking for an instance (instead of a class, for example):
>
>  if(!serializerSet.contains(s))
>
> Thus, every time a new serializer instance is registered, the
> serializerSet
> and serializerList grow, even if there was already a serializer
> instance of
> that class. In that case, the correct behaviour (at least that's what I
> think) should be replace the old instance with the new one.

We didn't yet have a case where we wanted to replace a serializer.

One option is to use the existing setSerializer() method on the Bridge
and give it a new JSONSerializer instance with your stateful serializers
- this problem should then go away.

Another way around would be to hold a reference to your custom
serializer and instead of re-registering it, update it's state.

> With this logic it's basically impossible to re-register a new
> serializer:
> if the passed instance is the same that the one already registered
> then it
> won't pass the contains check, and won't be registered; if (like in my
> case)
> the passed serializer is a completely new instance then it will be
> registered but the old one will remain in the serializerSet and
> serializerList. This currently works because the new instance is added to
> the beginning of the list, and that's the place where we start to look
> for
> serializers in the getSerializer method. The problem then is that these
> objects (the list and the set) will be always growing, and performance
> will
> degrade every time.
>

unregsiterSerializer(Serializer s) sounds like a good idea then (as
there could be cases where you register multiple instances of a
Serializer class configured in a different ways). The current interface
takes an instance not a class so a symmetrical interface (in line with
the current unregister*() methods) would be to remove the instance.

> My proposal is to replace the serializerSet with a Map
> (serializerMap), that
> indexes the serializers by class, keeping the serializer instance as the
> value of that entry. The instance can then be used to avoid
> re-registering
> the same instance twice, but will still enable registering new
> instances and
> removing the old one, keeping the serializerList containing only those
> serializers that are useful.
>
>    private HashMap serializerMap = new HashMap();
>
>    public void registerSerializer(Serializer s)
>    throws Exception
>    {
>    Class classes[] = s.getSerializableClasses();
>    Serializer exists;
>    synchronized (serializerMap) {
>        for(int i=0; i < classes.length; i++) {
>        exists = (Serializer)serializableMap.get(classes[i]);
>        if(exists != null && exists.getClass() != s.getClass())
>            throw new Exception
>            ("different serializer already registered for " +
>             classes[i].getName());
>        }
>
>        if(serializerMap.get(s.getClass()) != s) {
>        if(isDebug())
>            log.info("registered serializer " +
>                 s.getClass().getName());
>        for(int i=0; i < classes.length; i++) {
>            serializableMap.put(classes[i], s);
>        }
>        s.setOwner(this);
>        Object old = serializerMap.put(s.getClass(), s);
>        serializerList.remove(old);
>        serializerList.add(0, s);
>        }
>    }
>    }
>
> The only caveat I see is that the current code lets you register two
> serializer instances of the same class, each one being able to
> serialize a
> different set of classes. With the proposed change that would not
> work; and
> probably, to account for this case, it'll be necessary to update the
> serializableMap, removing the mappings of the old serializer prior to
> adding
> those for the new one.

Yes. I'm not sure if anybody is using that but my suggestion would be an
unregisterSerializer as per the other interfaces.

> Please let me know if this makes sense, or the current behavior is
> as-design. In such case I guess my solution would be to create a new
> JSONSerializer instance every time I need to re-register a serializer
> because my contextual information changed?

Yup. or hold references to your custom serializer and update its state
directly without re-registering it.

Michael.