jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Parvulescu <alex.parvule...@gmail.com>
Subject Re: SegmentRootBuilder#getNodeState creates a new revision on each call
Date Thu, 31 Oct 2013 15:54:10 GMT
hi,



On Thu, Oct 31, 2013 at 3:46 PM, Jukka Zitting <jukka.zitting@gmail.com>wrote:

> Hi,
>
> On Thu, Oct 31, 2013 at 5:23 AM, Alex Parvulescu
> <alex.parvulescu@gmail.com> wrote:
> > If the SegmentRootBuilder#getNodeState creates a new revision (even
> though
> > looking  at the apis this should not have side effects), this means that
> > any call basically invalidates all existing NodeBuilders and forces an
> > update on the entire tree.
>
> You misunderstand the effect of the method. What the getNodeState()
> method is called, the SegmentRootBuilder just flushes the current set
> of transient changes (since the last getNodeState call) to an
> underlying segment and returns the resulting SegmentNodeState. The
> journal is not updated, so this operation has no effect on any other
> NodeBuilders.
>

Fair enough, please take a look at [0], then [1]. this leads me to believe
that the base revision is increased on each call, so the other builders
would get invalidated (as mentioned on the #set method itself).
On the query benchmark the revision goes above one thousand, even if it's
basically a read-only test.



> > I'd like to use OAK-1130 as an example again. The trees are being loaded
> > around 5 times for each result row and it does look like there's
> something
> > in between that makes the #update call necessary even though the query
> test
> > itself should only have read calls against trees that have not changed
> > state at all.
>
> The repeated path lookups are unnecessary. See my comments on OAK-1130.
>

Yes, I fully agree with you on the lookups, I was talking about a potential
revision increase which might trigger some unneeded #update calls.


>
> > There is an important number of #getNodeState calls (both on Tree and on
> > NodeBuilder) so I'd be curious to see what others think about this issue.
>
> I think generally there shouldn't be too many cases where one would
> need to call getNodeState() on a builder. For example in the query
> engine I think using getBaseState() would be more appropriate, see the
> patch below.
>
>
Agreed again. See OAK-1132. I proposed the same idea, use the base state,
but there are some failing tests, other than the ones that you included in
this patch :)
But this is a different issue that should be tackled as a part of OAK-1132.

thanks,
alex

[0]
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/segment/SegmentRootBuilder.java?view=markup#l46
[1]
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/memory/MemoryNodeBuilder.java?view=markup#l236



> BR,
>
> Jukka Zitting
>
> diff --git
> a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
> b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
> index f3bfe31..f683508 100644
> ---
> a/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
> +++
> b/oak-core/src/main/java/org/apache/jackrabbit/oak/core/AbstractRoot.java
> @@ -328,15 +328,12 @@ public abstract class AbstractRoot implements Root {
>          return new QueryEngineImpl() {
>              @Override
>              protected ExecutionContext getExecutionContext() {
> -                NodeState rootState = AbstractRoot.this.getRootState();
> -                return new ExecutionContext(rootState, rootTree,
> getIndexProvider(rootState));
> -            }
> -
> -            private QueryIndexProvider getIndexProvider(NodeState
> rootState) {
> +                QueryIndexProvider provider = indexProvider;
>                  if (hasPendingChanges()) {
> -                    return new
> UUIDDiffIndexProviderWrapper(indexProvider, getBaseState(),
> rootState);
> +                    provider = new UUIDDiffIndexProviderWrapper(
> +                            provider, getBaseState(), getRootState());
>                  }
> -                return indexProvider;
> +                return new ExecutionContext(getBaseState(), rootTree,
> provider);
>              }
>          };
>      }
> diff --git
> a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/index/TraversingIndexQueryTest.java
>
> b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/index/TraversingIndexQueryTest.java
> index 6c5322c..e8ecf1b 100644
> ---
> a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/index/TraversingIndexQueryTest.java
> +++
> b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/index/TraversingIndexQueryTest.java
> @@ -52,6 +52,7 @@ public class TraversingIndexQueryTest extends
> AbstractQueryTest {
>          //OAK-1024 allow '/' in a full-text query
>          Tree node = root.getTree("/").addChild("content");
>          node.setProperty("jcr:mimeType", "text/plain");
> +        root.commit();
>          assertQuery("//*[jcr:contains(., 'text/plain')]", "xpath",
>                  ImmutableList.of("/content"));
>      }
> @@ -61,6 +62,7 @@ public class TraversingIndexQueryTest extends
> AbstractQueryTest {
>          Tree c = root.getTree("/").addChild("content");
>          c.addChild("testFullTextTermNameSimple");
>          c.addChild("testFullTextTermNameFile.txt");
> +        root.commit();
>          assertQuery("//*[jcr:contains(., 'testFullTextTermNameSimple')]",
>                  "xpath",
>                  ImmutableList.of("/content/testFullTextTermNameSimple"));
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message