Dropping ItemImpl.checkPropertected()

[Jukka Zitting <ju...@gmail.com>]

Hi,

While doing some basic benchmarking of transient addNode() and
setProperty() calls I noticed that almost half of the time taken by
those methods goes into the checkProtected() calls that looks up the
relevant item definition and verifies that the item in question is not
protected.

We could optimize that check a lot by explicitly keeping track of
effective node types and item definitions (like what jackrabbit-core
does), but I'd rather avoid the extra overhead unless we have a really
compelling reason for why this should be done before save(), since in
any case we'll be enforcing such rules in the commit hooks.
Furthermore no existing client code should be broken by such a change
since there's hardly code out there that explicitly tries to modify
protected items

I tried disabling the protection checks and the only TCK test that
fails is  NodeTest.testPrimaryTypeProtected(). I'd declare that as a
known issue and rather have the relevant check postponed to save()
where a validator can take care of it.

Thoughts on this? Unless anyone has strong objections, I'd suggest we
simply drop the checkProtected() calls and have the relevant
constraints enforced during save().

BR,

Jukka Zitting

Re: Dropping ItemImpl.checkPropertected()

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Feb 26, 2013 at 1:11 PM, Angela Schreiber <an...@adobe.com> wrote:
> usually the specification explicitly states if a given validation
> may be postponed until the save-call which is not the case here.

I think we should probably suggest to JSR 333 to relax that
constraint, but until we do I guess you're right that sticking with
the strict interpretation is best. I'll look into optimizing the
relevant node type code.

BR,

Jukka Zitting

Re: Dropping ItemImpl.checkPropertected()

[Angela Schreiber <an...@adobe.com>]

i am not totally sure if the checkProtection can be delayed until
the save call. the specification states:

"If an item I is declared protected it is repository-controlled.
  If I is a node then, through the core write methods of JCR (see
  §10.2 Core Write Methods),
• I cannot be removed,
• child nodes of I cannot be added, removed, or reordered,
• properties of I cannot be added or removed,
• the values of existing properties of I cannot be changed,
• the primary node type of I cannot be changed and
• mixin node types cannot be added to or removed from I.
If I is a property then, through the core write methods of JCR (see 
§10.2 Core
Write Methods),
• I cannot be removed and
• the value of I cannot be changed.
"

usually the specification explicitly states if a given validation
may be postponed until the save-call which is not the case here.

kind regards
angela





On 2/26/13 11:25 AM, Jukka Zitting wrote:
> Hi,
>
> On Tue, Feb 26, 2013 at 11:08 AM, Marcel Reutegger<mr...@adobe.com>  wrote:
>> I'd be OK to replace the full-fledged checked in each call with a simple
>> hardcoded set of well-known protected properties. the full check on
>> save() would then take care of the rest.
>
> If we do keep the check in the transient operations like addNode and
> setProperty, then I think it's best to do the check properly to avoid
> weird corner cases (like whether jcr:uuid should be protected in
> non-referenceable nt:unstructured nodes). It should be possible to
> optimize the checkProtected() method down to microsecond range if we
> go that route.
>
> My main point here is whether such optimization is needed in the first
> place since these checks are just duplicating effort that in any case
> needs to be done during save().
>
> BR,
>
> Jukka Zitting

Re: Dropping ItemImpl.checkPropertected()

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Feb 26, 2013 at 11:08 AM, Marcel Reutegger <mr...@adobe.com> wrote:
> I'd be OK to replace the full-fledged checked in each call with a simple
> hardcoded set of well-known protected properties. the full check on
> save() would then take care of the rest.

If we do keep the check in the transient operations like addNode and
setProperty, then I think it's best to do the check properly to avoid
weird corner cases (like whether jcr:uuid should be protected in
non-referenceable nt:unstructured nodes). It should be possible to
optimize the checkProtected() method down to microsecond range if we
go that route.

My main point here is whether such optimization is needed in the first
place since these checks are just duplicating effort that in any case
needs to be done during save().

BR,

Jukka Zitting

RE: Dropping ItemImpl.checkPropertected()

[Marcel Reutegger <mr...@adobe.com>]

Hi,

> On Tue, Feb 26, 2013 at 10:14 AM, Angela Schreiber <an...@adobe.com>
> wrote:
> > -1 from my side. i think it's important to distinguish protected
> > items that are meant to be filled in by the system or by dedicated API.
> 
> Agreed, but do we need to make this distinction separately during each
> addNode() and setProperty() call, instead of just once during save()?

I'd be OK to replace the full-fledged checked in each call with a simple
hardcoded set of well-known protected properties. the full check on
save() would then take care of the rest. basically for node types, which
are not provided by the repository, but the application. I'd even argue
there cannot be any application defined node type definitions with
protected items...

Regards
 Marcel

Re: Dropping ItemImpl.checkPropertected()

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Feb 26, 2013 at 10:14 AM, Angela Schreiber <an...@adobe.com> wrote:
> -1 from my side. i think it's important to distinguish protected
> items that are meant to be filled in by the system or by dedicated API.

Agreed, but do we need to make this distinction separately during each
addNode() and setProperty() call, instead of just once during save()?

BR,

Jukka Zitting

Re: Dropping ItemImpl.checkPropertected()

[Angela Schreiber <an...@adobe.com>]

-1 from my side. i think it's important to distinguish protected
items that are meant to be filled in by the system or by dedicated API.

if the evaluation is slow we should optimize it.

kind regards
angela

On 2/25/13 2:17 PM, Jukka Zitting wrote:
> Hi,
>
> While doing some basic benchmarking of transient addNode() and
> setProperty() calls I noticed that almost half of the time taken by
> those methods goes into the checkProtected() calls that looks up the
> relevant item definition and verifies that the item in question is not
> protected.
>
> We could optimize that check a lot by explicitly keeping track of
> effective node types and item definitions (like what jackrabbit-core
> does), but I'd rather avoid the extra overhead unless we have a really
> compelling reason for why this should be done before save(), since in
> any case we'll be enforcing such rules in the commit hooks.
> Furthermore no existing client code should be broken by such a change
> since there's hardly code out there that explicitly tries to modify
> protected items
>
> I tried disabling the protection checks and the only TCK test that
> fails is  NodeTest.testPrimaryTypeProtected(). I'd declare that as a
> known issue and rather have the relevant check postponed to save()
> where a validator can take care of it.
>
> Thoughts on this? Unless anyone has strong objections, I'd suggest we
> simply drop the checkProtected() calls and have the relevant
> constraints enforced during save().
>
> BR,
>
> Jukka Zitting

Re: Dropping ItemImpl.checkPropertected()

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Mon, Feb 25, 2013 at 3:17 PM, Jukka Zitting <ju...@gmail.com> wrote:
> Thoughts on this? Unless anyone has strong objections, I'd suggest we
> simply drop the checkProtected() calls and have the relevant
> constraints enforced during save().

I filed https://issues.apache.org/jira/browse/OAK-652 to track the
implementation of whichever consensus we find on this, to remore or to
optimize the checkProtected() call.

Meanwhile I'll add a temporary feature flag to allow further
benchmarking without having this issue mask other potential problems.

BR,

Jukka Zitting