memory optimization ( Constraint instances )

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

Ok, so as I see the API

Code:

public interface Constraint<A extends Annotation> {
   void initialize(A constraintAnnotation);
   boolean isValid(Object value);
}

it looks like the validation logic should instantiate Constraint for each corresponding annotation (constraint descriptor) found into the domain model.

Let's take a look at two example Constraint-s:

(1)

Code:

public class MaxValidator {

   private long max;

   public void initialize(Max parameters) {
      max = parameters.value();
   }

   public boolean isValid(Object value) {
      //...
   }
}

(2)

Code:

public class PatternValidator implements Validator<Pattern>, Serializable {

   private java.util.regex.Pattern pattern;

   public void initialize(Pattern parameters) {
      pattern = java.util.regex.Pattern.compile(
         parameters.regex(),
         parameters.flags()
      );
   }

   public boolean isValid(Object value) {
      //...
   }
}

So in (2) we have a time consuming initialization, the API behaviour makes sense.

But in (1) we do nothing - just copy some information from the constraint descriptor in our Constraint instance.

So, what about having two kinds of Constraints?
The other one can look like this:

Code:

public interface SimpleConstraint<A extends Annotation> {
   boolean isValid(Object value, A constraintAnnotation);
}

Don't know if the name SimpleConstraint is a good one, but anyway for any Constraint which does nothing special into its initialize(...) method it will be better to not be initialized on a per annotation basis. A single instance could be enough, or instance per a context.

This will be usefull when executing in limited memory environments.
What do you think?

emmanuel · **Posted:** Wed Apr 23, 2008 8:18 pm

Does the memory saved really worth the API ugliness?

In the case of Max, you save probably not more than 100 bytes.

Note that the spec allows to share the same MaxConstraintImpl for the same @Max "values"
In practice, I think nobody will implement that as it will cost more CPU than it worth of memory saving.

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

Quote:

Does the memory saved really worth the API ugliness?

Is there any ugliness at all?
I think the ugliness is that you instantiate classes when you actually do not need to do it.
Consider using the API in mobile application development for example..

Quote:

In the case of Max, you save probably not more than 100 bytes.

The point is that you do not need more than one instance of Max, because it can use already instantiated constraint annotation..

Quote:

Note that the spec allows to share the same MaxConstraintImpl for the same @Max "values"

And how could you share a single MaxConstraintImpl instance in a multithreaded environment?

Quote:

In practice, I think nobody will implement that as it will cost more CPU than it worth of memory saving.

Don't think so. Actually it will save also the CPU, because you will have only one "new MaxConstraintImpl()" and you will call one method when you are using it (isValid(Object value, A constraintAnnotation)) instead of calling two methods one after another (initialize and isValid(Object value)).

emmanuel · **Posted:** Thu Apr 24, 2008 10:16 am

bodrin wrote:

Quote:

Note that the spec allows to share the same MaxConstraintImpl for the same @Max "values"

And how could you share a single MaxConstraintImpl instance in a multithreaded environment?

The spec contract requires isValid to be thread safe.

bodrin wrote:

Quote:

In practice, I think nobody will implement that as it will cost more CPU than it worth of memory saving.

Don't think so. Actually it will save also the CPU, because you will have only one "new MaxConstraintImpl()" and you will call one method when you are using it (isValid(Object value, A constraintAnnotation)) instead of calling two methods one after another (initialize and isValid(Object value)).

We don't care about the new and initialize calls they are done once per annotation at init time. isValid is called a lot and a lot when the app is up and running.

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

Quote:

...
The spec contract requires isValid to be thread safe.

Ok, but if you make isValid synchronized somehow this will lead to performance degradation.
The point is that the SimpleConstraint does not have to keep state (the state is kept into the constraint annotation instance), so this makes it thread safe by design.

Quote:

...
We don't care about the new and initialize calls they are done once per annotation at init time. isValid is called a lot and a lot when the app is up and running.

So, the validation time is the same for (1) and (2), but the initialization time is improved with SimpleConstraint (when used properly)!

To summarize I can say that when used properly the proposed extension :
-improves the performance (CPU usage during init time)
-improves the memory usage (we do care about this at least in some environments)
-does not break code manageability

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

Look at the HibernateValidator constraint implementations:

I. SimpleConstraint (14)
A) with empty initialize method (8)

Code:

// 1
public class AssertFalseValidator implements Validator<AssertFalse>, Serializable {
    public void initialize(AssertFalse parameters) {
    }
}

// 2
public class AssertTrueValidator implements Validator<AssertTrue>, Serializable {
    public void initialize(AssertTrue parameters) {
    }
}

// 3
public class CreditCardNumberValidator extends AbstractLuhnValidator implements Validator<CreditCardNumber>, Serializable {
    public void initialize(CreditCardNumber parameters) {
    }
}

// 4
public class EANValidator implements Validator<EAN> {
    public void initialize(EAN parameters) {
    }
}

// 5
public class FutureValidator implements Validator<Future>, Serializable {
    public void initialize(Future parameters) {
    }
}

// 6
public class NotEmptyValidator implements Validator<NotEmpty>, PropertyConstraint, Serializable {
    public void initialize(NotEmpty parameters) {
    }
}

// 7
public class NotNullValidator implements Validator<NotNull>, PropertyConstraint, Serializable {
    public void initialize(NotNull parameters) {
    }
}

// 8
public class PastValidator implements Validator<Past>, PropertyConstraint, Serializable {
    public void initialize(Past parameters) {
    }
}

B) initialize method copying data from the Annotation (6)

Code:

// 1
public class DigitsValidator implements Validator<Digits>, PropertyConstraint {
    public void initialize(Digits configuration) {
        integerDigits = configuration.integerDigits();
        fractionalDigits = configuration.fractionalDigits();
    }
}

// 2
public class LengthValidator implements Validator<Length>, PropertyConstraint, Serializable {
    public void initialize(Length parameters) {
        max = parameters.max();
        min = parameters.min();
    }
}

// 3
public class MaxValidator implements Validator<Max>, PropertyConstraint, Serializable {
    public void initialize(Max parameters) {
        max = parameters.value();
    }
}

// 4
public class MinValidator implements Validator<Min>, PropertyConstraint, Serializable {
    public void initialize(Min parameters) {
        min = parameters.value();
    }
}

// 5
public class RangeValidator implements Validator<Range>, PropertyConstraint, Serializable {
    public void initialize(Range parameters) {
        max = parameters.max();
        min = parameters.min();
    }
}

// 6
public class SizeValidator implements Validator<Size>, Serializable {
    public void initialize(Size parameters) {
        max = parameters.max();
        min = parameters.min();
    }
}

II. Constraint (2)

Code:

// 1
public class EmailValidator implements Validator<Email>, Serializable {
    public void initialize(Email parameters) {
        pattern = java.util.regex.Pattern.compile(
                "^[_A-Za-z0-9-]+(\\.[_A-Za-z0-9-]+)*@[A-Za-z0-9-]+(\\.[A-Za-z0-9-]+)*$",
                java.util.regex.Pattern.CASE_INSENSITIVE
        );
        pattern = java.util.regex.Pattern.compile(
                "^" + ATOM + "+(\\." + ATOM + "+)*@"
                         + DOMAIN
                         + "|"
                         + IP_DOMAIN
                         + ")$",
                java.util.regex.Pattern.CASE_INSENSITIVE
        );
    }
}

// 2
public class PatternValidator implements Validator<Pattern>, Serializable {
   public void initialize(Pattern parameters) {
        pattern = java.util.regex.Pattern.compile(
                parameters.regex(),
                parameters.flags()
        );
    }
}

So we have Constraint : SimpleConstraint = 2 : 14 = 1 : 7.
I thing we shoud reverse the naming, because the SimpleConstraint is used more often than the Constraint:
SimpleConstraint -> Constraint
Constraint -> HeavyweightConstraint

Maybe you do not like the interface name HeavyweightConstraint.., but I hope you get the idea.

sanne.grinovero · **Posted:** Tue Apr 29, 2008 6:28 am

Quote:

but I hope you get the idea.

I'm sorry, I didn't understand what you are proposing; would you like to explain this numbers?

sanne.grinovero · **Posted:** Tue Apr 29, 2008 6:35 am

IMHO the way some framework does it's internal optimization doesn't need to be standardized, implementors should be free to think something smart.

A good hint to write down in the standard could be this:
"if two different validators return true on a equals() test, then the same instance may be reused"

So people could get his own validators reused implementing equals+hashcode based on it's instance variables (configuration options) and class type.

This means that some implementor could optimize it easily, putting all validators it is initialyzing in a set to verify duplicates, and then throwing away the set when all initialization is complete; Also this implementation detail would be optional, so if the overhead of duplicates checking on startup is too high it could be left out.

what do you think of this proposal?

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

What I'm trying to say is that obviously the more common case is the one when you do not need a separate initialize() method on the constraint implementation (14 hibernate validators above), because no initialization is needed.
This method is empty in 8 of the hibernate validators and in 6 of them it just copies data from the annotation.
So, it is better to skip the initialization step for all these cases and have just one method:

Code:

public interface SimpleConstraint<A extends Annotation> {
   boolean isValid(Object value, A constraintAnnotation);
} 

This way we need only one instance of the validator implementation.

The other case is when you need the initialization step - you are doing some calculation (java.util.regex.Pattern.compile..), saving the result into validator instance variables in order to be used when isValid from

Code:

public interface Constraint<A extends Annotation> {
   void initialize(A constraintAnnotation);
   boolean isValid(Object value);
}

is called (used only in 2 of the hibernate validators).
In this case you need an instance of the validator implementation per each annotation instance in order to save the initialization calculations and use them later when you validate.

In the current API this two cases are mixed. I want to propose to split them.

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

Ok, so here is the thought chain about how we refactor the current API:

Code:

public interface Constraint<A extends Annotation> {
   void initialize(A constraintAnnotation);
   boolean isValid(Object value);
} 

in this direction:

1) We do not need to copy the information from the constrainAnnotation in initialize(...), we are just using it to calculate and save some data, so that we can use it later when we validate. But when we validate we may need some information from the constraintAnnotation:

Code:

public interface Constraint<A extends Annotation> {
   void initialize(A constraintAnnotation);
   boolean isValid(Object value, A constraintAnnotation);
} 

We just pass it again.

2) We want to support non-initializable constraints in order to skip the redundant constrain implementation instantiation. In order to achieve this we split the interface this way:

Code:

public interface Constraint<A extends Annotation> {
   boolean isValid(Object value, A constraintAnnotation);
} 

Code:

public interface Initializable<A extends Annotation> {
   void initialize(A constraintAnnotation);
} 

So, each constraint implementation implements the interface Constraint and if it need initializatino it also implements Initializable.
Then validation framework checks wether our Constraint is initializable and uses it accordingly.

dwaite · **Joined:** Sun Mar 30, 2008 12:19 am **Posts:** 15

I don't know if having a 'SimpleConstraint' with a single initialize+validate method would actually save memory, as another class instance might be made necessary to hold either

- the tuple of SimpleConstraint + Annotation
- a HeavyweightConstraint

for the validator cache.

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

Quote:

- the tuple of SimpleConstraint + Annotation

We don't need this, because we already have the mapping :

Code:

/**
* Link between an constraint annotation and its constraint validation implementation
*
...
public @interface ConstraintValidator {
...

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

ok, may be it is better to change the interface name Initializable to InitializableConstraint or even better to ExtendedConstraint and make it extend the base constraint interface:

Code:

public interface Constraint<A extends Annotation> {
   boolean isValid(Object value, A constraintAnnotation);
}

Code:

public interface ExtendedConstraint<A extends Annotation> extends Constraint<A> {
   void initialize(A constraintAnnotation);
}

bodrin · **Joined:** Thu Apr 17, 2008 5:47 pm **Posts:** 26

Also I have found that the statistic above 14:2 is actually 15:1 (non-initializable constraints : initializable constraints),
because the hibernate EmailValidator does not need initialization at all.

It is clear that the email regular expression should be a static one or lazy initialized if you preffer.

I think it is clear that in practice we need initializable constraints very rarely and it is good to make the model be aware of that fact.

emmanuel · **Posted:** Mon Jun 02, 2008 6:02 pm

bodrin wrote:

Quote:

...
The spec contract requires isValid to be thread safe.

Ok, but if you make isValid synchronized somehow this will lead to performance degradation.
The point is that the SimpleConstraint does not have to keep state (the state is kept into the constraint annotation instance), so this makes it thread safe by design.

You can be thread-safe and have state without requiring synchronization as state is prepared at init time.

bodrin wrote:

Quote:

- the tuple of SimpleConstraint + Annotation

We don't need this, because we already have the mapping :

Code:

/**
* Link between an constraint annotation and its constraint validation implementation
*
...
public @interface ConstraintValidator {
...

I think you need to keep the tuple for two reasons:
- things can be set up by XML. so you somehow need to keep the metadata around. You won't save memory.
- reading annotations through the reflection API calls a synchronized method. We had to adjust Seam to avoid this bottleneck.