Google Groups

Re: [orientdb] Re: Released OrientDB v1.0rc5: improved index and transactions, better crossing of trees and graphs


Lvc@ Oct 10, 2011 7:07 AM
Posted in group: OrientDB
Hi Adolfo,
good analysis! You could create a static helper class called something like OCloneHelper and put all this stuff here to avoid to make the Gremlin function too big and to let to other part of the code to use it in the future.

Lvc@

On 10 October 2011 15:50, Adolfo Rodriguez <pelly...@yahoo.es> wrote:
I have been reading and, as far as I understood, there are several
possible copy/clone mechanisms (all of them as workarounds of
http://bugs.sun.com/view_bug.do?bug_id=4098033 since there is NOT  an
unique, low footprint, polymorphic clone in Java). Namely:
* Class by class clone (instanceof):
      1. use copy constructor (the one used above)
      2. use implemented clone method (only shallow clone)
* Polimorphic clone:
      3. reflection on clone (polimorphic clone)
      4 ((Cloneable)object).clone(), not possible since clone is
protected
      5. Serialize deserialize -> would be the cleanest but has a big
performance overhead

I have not investigated third party libraries.

Polymorphic cloning would have the advantage of being agnostic to the
object type, so would be the cleanest code. However, these methods
have a big performance overhead. Therefore I propose a 3 levels
procedure like:
1* using instanceof for the list of known clonable classes (done in
previous code).
2* If object not on the instanceof list, try reflection on clone()
method and notify user about possible improvement in performance
(ignore, fix now or open a low priority issue)
3* if reflection does not find clone() implementation, try
serialization and notify user about possible improvement in
performance (ignore, fix now or open a low priority issue). This is
the most powerful but the more expensive.

In this way ALL TYPES OF OBJECTS WOULD BE COVERED but optionally code
can be enhanced to improve performance for a known type required (by
adding it to the set of instanceof's).

I have created such a code with 3 levels of cloning (1.instanceof,
2.reflection and 3.serialization) but I do not want to overwhelm you
with this stupid thing.

How do you want to address the issue?

Adolfo

On Oct 10, 3:25 pm, Luca Garulli <l.garu...@gmail.com> wrote:
> Hi Adolfo,
> you're right, I just wrote down that piece of code without too many checks.
>
> Lvc@
>
> On 10 October 2011 15:23, Adolfo Rodriguez <pellyado...@yahoo.es> wrote:
>
>
>
>
>
>
>
> > Hi Luca,
>
> > I am a bit confused with previous code. It would never be possible to
> > pass a preloaded map from the client. It would be always cleared by
>
> > else{
> >          ((Map)value).clear();
>
> > Is not it?
>
> > -----------------------------------------------------
>
> > To recycle the variable I would do something as:
>
> > if (value instanceof Map) {
> >        Map oldMap = (Map) clonedParameters.get(param.getKey());
> >        if (oldMap == null)
> >                oldMap = new HashMap();  // first time
> >        else oldMap.clear();
> >        oldMap.putAll((Map)value);
> >        clonedParameters.put(param.getKey(), oldMap);
> > }
>
> > There are other options (reflection, serialization...) but I do not
> > state here to simplify, at the moment.
>
> > Unfortunately clone is one of the worst things in Java as it has bugs
> > since first releases of the JVM which has not been fixed to keep
> > compatibility (http://bugs.sun.com/view_bug.do?bug_id=4098033).
>
> > Adolfo
>
> > On Oct 10, 9:25 am, Luca Garulli <l.garu...@gmail.com> wrote:
> > > Hi Adolfo,
> > > in this last post you don't reuse the same object anymore, while in the
> > > previous one you had a "clone" variable.
>
> > > public Map<Object, Object> getParameters() {
> > >        if (parameters == null)
> > >                return null;
>
> > >        clonedParameters.clear();
> > >        for (Entry<Object, Object> param : parameters.entrySet()) {
> > >                Object value = param.getValue();
> > >                if (value instanceof Map) {
> > >                      if( ((Map)value).isEmpty() )
> > >                        clonedParameters.put(param.getKey(), new
> > > HashMap((Map)value));
> > >                      else{
> > >                        ((Map)value).clear();
> > >                        clonedParameters.put(param.getKey(), value);
> > >                      }
> > >                } else if (value instanceof Collection) {
> > >                      if( ((Collection)value).isEmpty() )
> > >                        clonedParameters.put(param.getKey(), new
> > > ArrayList((List)value));
> > >                      else{
> > >                        ((Collection)value).clear();
> > >                        clonedParameters.put(param.getKey(), value);
> > >                      }
> > >                } else if (value instanceof String) {
> > >                        clonedParameters.put(param.getKey(), value);
> > >                }
> > >                else System.out.println(value.getClass() + " missed
> > type");
> > >        }
> > >        return clonedParameters;
>
> > > }
>
> > > WDYT?
>
> > > Lvc@
>
> > > On 9 October 2011 03:21, Adolfo Rodriguez <pellyado...@yahoo.es> wrote:
>
> > > > Hi,
>
> > > > I think this looks much better. It would fix the issue (if there are
> > > > not any bugs). It just keeps the original Map as a pattern for
> > > > comparison in all iterations and refresh second map with the pattern
> > > > values for each iteration. Nothing else is needed. When there are not
> > > > arguments it just returns an unexpensive null. In
> > > > OCommandExecutorAbstract:
>
> > > > protected Map<Object, Object>   parameters;
> > > > protected Map<Object, Object>   clonedParameters = new HashMap<Object,
> > > > Object>();
>
> > > > [...]
>
> > > > public Map<Object, Object> getParameters() {
> > > >         if (parameters == null)
> > > >                return null;
>
> > > >        clonedParameters.clear();
> > > >         for (Entry<Object, Object> param : parameters.entrySet()) {
> > > >                Object value = param.getValue();
> > > >                if (value instanceof Map) {
> > > >                         clonedParameters.put(param.getKey(), new
> > > > HashMap((Map)value));
> > > >                 } else if (value instanceof Collection) {
> > > >                         clonedParameters.put(param.getKey(), new
> > > > ArrayList((List)value));
> > > >                 } else if (value instanceof String) {
> > > >                         clonedParameters.put(param.getKey(), value);
> > > >                 }
> > > >                else System.out.println(value.getClass() + " missed
> > type");
> > > >        }
> > > >         return clonedParameters;
> > > > }
>
> > > > Adolfo
>
> > > > On Oct 9, 12:08 am, Adolfo Rodriguez <pellyado...@yahoo.es> wrote:
> > > > > * there is also a risk with pointers so the final solution needs
> > tests
> > > > > * clone() provided by ArrayList and Map by default is a shallow
> > clone.
> > > > > To get a deep clone it must be done manually.
>
> > > > > On Oct 8, 11:59 pm, Adolfo Rodriguez <pellyado...@yahoo.es> wrote:
>
> > > > > > Hi Luca, not real value provided in this post but, for your
> > reference,
> > > > > > code below works:
>
> > > > > > public Map<Object, Object> getParameters() {
> > > > > >         boolean clone = true;
>
> > > > > >         Map<Object, Object> clonedParameters = new HashMap<Object,
> > > > Object>();
> > > > > >         for (Entry<Object, Object> param : parameters.entrySet()) {
> > > > > >                 Object value = param.getValue();
> > > > > >                 if (value instanceof Map) {
> > > > > >                         if (clone)
> > > > > >                                 value = new HashMap<String,
> > > > String>((Map<String, String>)value);
> > > > > >                         else ((Map<String, String>)value).clear();
> > > > > >                 } else if (value instanceof Collection) {
> > > > > >                         if (clone)
> > > > > >                                 value = new
> > > > ArrayList<String>((Collection<String>)value);
> > > > > >                         else ((Collection<String>)value).clear();
> > > > > >                 }
> > > > > >                 else if (value instanceof String) { /* do nothing
> > */ }
> > > > > >                 //else System.out.println(value.getClass() + "
> > missed
> > > > type");
>
> > > > > >                 clonedParameters.put(param.getKey(), value);
> > > > > >         }
> > > > > >         return clonedParameters;
>
> > > > > >         //return parameters;
>
> > > > > > }
>
> > > > > > * The ideal thing would be passing by value the whole parameters
> > Map
> > > > > > but there is such a mechanism in Java.
> > > > > > * Also I considered, that OCommandExecutorAbstract contains the
> > > > > > serialized Objects passed from client (I suppose they are
> > serialized
> > > > > > when passed by the channel) and deserializing once per execute()
> > call,
> > > > > > but I am afraid of the performance overhead.
> > > > > > * Code above does not seen to degrade performance in my tests. I
> > > > > > assume only simple Objects are passed. I have not considered Date
> > yet.
> > > > > > * Remaining to calculate the value of 'boolean clone' which I think
> > > > > > needs a variable per collection/map and at the same time that
> > > > > > parameters variable is filled
>
> > > > > > I cannot figure out anything more elegant at the moment. Can you?
>
> > > > > > Take your time, I can carry on with a fix in the meantime.
>
> > > > > > Adolfo
>
> > > > > > On Oct 8, 5:26 pm, Adolfo Rodriguez <pellyado...@yahoo.es> wrote:
>
> > > > > > > Yes, you are right. External configuration is not required for
> > clone
> > > > > > > vs clear. I agree.
>
> > > > > > > >> if maps/collections arguments are not empty
> > > > > > > >> they could be cloned
>
> > > > > > > .............are not empty............ *on the first execution*.
> > Mind
> > > > > > > that, in the second execution they can have been already
> > populated by
> > > > > > > the first one, even when they where propagated empty from client.
> > So
> > > > > > > it must be checked before the loop of calls to execute().
>
> > > > > > > Therefore:
> > > > > > > * If is preloaded the first time, a fresh clone of variables is
> > > > needed
> > > > > > > for each call to execute().
> > > > > > > * If is empty the first time, just clear on every call (saving
> > the
> > > > > > > Instantiation time - assuming that clearing is lighter than
> > > > > > > instantiating (?)).
>
> > > > > > > Yes, this would work. I do not if this would impact badly
> > somewhere
> > > > > > > else, but I think rationale would be the same.
>
> > > > > > > Adolfo
>
> > > > > > > On Oct 8, 5:10 pm, Luca Garulli <l.garu...@gmail.com> wrote:
>
> > > > > > > > Yeah,
> > > > > > > > it could be easy to handle it: if maps/collections arguments
> > are
> > > > not empty
> > > > > > > > they could be cloned, otherwise just cleared every time. WDYT?
>
> > > > > > > > Lvc@
>
> > > > > > > > On 8 October 2011 14:35, Adolfo Rodriguez <
> > pellyado...@yahoo.es>
> > > > wrote:
>
> > > > > > > > > Hi Luca
>
> > > > > > > > > when you say "to pass a Map as argument", I suppose you mean
> > "to
> > > > pass
> > > > > > > > > a PRELOADED Map (with values) as argument". Yes, I have not
> > seen
> > > > this
> > > > > > > > > case in Gremlin but theoretically is possible. I was leaving
> > it
> > > > out to
> > > > > > > > > keep the changes local and adopt use cases incrementally.
>
> > > > > > > > > To consider also that case, the way would be cloning the
> > original
> > > > set
> > > > > > > > > of variables so every execution of Gremlin execute() gets a
> > fresh
> > > > copy
> > > > > > > > > of them.
>
> > > > > > > > > * Where? A proper place could be the
> > iRequester.getParameters();
> > > > > > > > > method of OCommandExecutorAbstract, or creating a second
> > method
> > > > > > > > > getClonedParameters(). Also, this can be done somewhere in
> > the
> > > > Gremlin
> > > > > > > > > function, if you think is a very specific use case.
>
> > > > > > > > > * When? either clone always or (clone vs clear) when the user
> > > > > > > > > configures it with another flag passed from the client.
>
> > > > > > > > > So the question would be how to clone at the lowest
> > performance
> > > > cost.
> > > > > > > > > However, in the 90% of cases, clear() would be the choice, I
> > > > think, so
> > > > > > > > > performance cost would be only on this 10%.
>
> > > > > > > > > WDYT?
>
> > > > > > > > > Adolfo
>
> ...
>
> read more »