Merge lp:~zorba-coders/zorba/plan-serializer into lp:zorba

[Daniel Turcanu]

Fix in plan serializer about preparing user_functions plan for serialization.

Fix in plan serializer about preparing user_functions plan for serialization.

[Markos Zaharioudakis (markos-za)]

I would like to understand this better. What is the problem that this change fixes?

[Daniel Turcanu (danielturcanu)]

The user_functions were compiled during plan serialization, and this modified some expr trees that were already serialized. I had to ensure that all user_functions are compiled before plan serialization is started.

[Markos Zaharioudakis (markos-za)]

> The user_functions were compiled during plan serialization, and this modified
> some expr trees that were already serialized. I had to ensure that all
> user_functions are compiled before plan serialization is started.

Yes, I understood that myself, but what exactly was modified after having been serialized?

[Daniel Turcanu (danielturcanu)]

For example in one test, in flwor_expr object, theClauses was receiving another flwor_clause in the vector, which made the vector to increase and move to another space. But the initial flwor_clause had already been serialized, and its address was registered for address duplication detection. Sometimes another object would be allocated at the same address, and the plan serializer thought it is the same object. This only occured sometimes, so the problem was hard to debug.
So as a rule, all the objects should freeze during plan serialization, nothing should change.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/plan-serializer-2012-01-18T10-22-58.412Z/log.html

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~danielturcanu/zorba/plan-serializer into lp:zorba failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:273 (message):
  Validation queue job plan-serializer-2012-01-18T10-22-58.412Z is finished.
  The final status was:

  No tests were run - build or configure step must have failed.

  Not commiting changes.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[Chris Hillery (ceejatec)]

Code does not compile. I would fix if this branch was owned by ~zorba-coders, but since it's ~danielturcanu, we'll need Daniel to fix it.

[ 13%] Building CXX object src/CMakeFiles/zorba_simplestore.dir/api/xqueryimpl.cpp.o
.In file included from ../../src/api/xqueryimpl.cpp:58:
../../src/compiler/api/compilercb.h:169: warning: '>>' operator will be treated as two right angle brackets in C++0x
../../src/compiler/api/compilercb.h:169: warning: suggest parentheses around '>>' expression
../../src/compiler/api/compilercb.h:169: error: 'theLocalUdfs' was not declared in this scope
../../src/compiler/api/compilercb.h:169: error: '>>' should be '> >' within a nested template argument list
../../src/compiler/api/compilercb.h:176: warning: '>>' operator will be treated as two right angle brackets in C++0x
../../src/compiler/api/compilercb.h:176: warning: suggest parentheses around '>>' expression
../../src/compiler/api/compilercb.h:176: error: a function call cannot appear in a constant-expression
../../src/compiler/api/compilercb.h:176: error: '>>' should be '> >' within a nested template argument list

[lp:~zorba-coders/zorba/plan-serializer]

10531. By Daniel Turcanu on 2012-01-18

Fix linux build

[Markos Zaharioudakis (markos-za)]

> For example in one test, in flwor_expr object, theClauses was receiving
> another flwor_clause in the vector, which made the vector to increase and move
> to another space. But the initial flwor_clause had already been serialized,
> and its address was registered for address duplication detection. Sometimes
> another object would be allocated at the same address, and the plan serializer
> thought it is the same object. This only occured sometimes, so the problem was
> hard to debug.
> So as a rule, all the objects should freeze during plan serialization, nothing
> should change.

I am sorry, but this still does not answer my question. I assume that the flwor_expr is inside the body of a udf. Are you saying that this body is somehow serialized, without having being optimized yet? If yes, I don't understand how this is possible, given that user_function::serialize() will first optimize the body (if not optimized already) before serializing it. Either I am missing something, or the real bug is somewhere else and you are just covering it up with this patch.

[Daniel Turcanu (danielturcanu)]

In my debugging I saw that the flwor_expr is outside the udf. So when compiling a udf, probably the optimizer is changing the expression tree in other places as well, probably the caller of the udf, but I cannot say for sure. This woulnd't surprise me, it should be normal for an optimizer.

[lp:~zorba-coders/zorba/plan-serializer]

10532. By Daniel Turcanu on 2012-01-30

Update to trunk

10533. By Daniel Turcanu on 2012-01-31

Fix Win build

10534. By Daniel Turcanu on 2012-01-31

Update to trunk

10535. By Daniel Turcanu on 2012-02-01

Update to trunk

10536. By Daniel Turcanu on 2012-02-01

Fix plan serializer for some full text classes

10537. By Matthias Brantner on 2012-02-01

fix for bug #924205

10538. By Daniel Turcanu on 2012-02-06

Update to trunk

Unmerged revisions

10538. By Daniel Turcanu on 2012-02-06

Update to trunk

=== modified file 'src/api/xqueryimpl.cpp'
--- src/api/xqueryimpl.cpp	2012-01-11 17:30:25 +0000
+++ src/api/xqueryimpl.cpp	2012-01-18 11:01:27 +0000
@@ -193,7 +193,8 @@
   }
   else
   {
-    ar.compiler_cb = theCompilerCB;
+    //ar.compiler_cb = theCompilerCB;
+    theCompilerCB->prepare_for_serialize();
   }
 
   ar & theCompilerCB;

=== modified file 'src/compiler/api/compilercb.cpp'
--- src/compiler/api/compilercb.cpp	2012-01-11 17:30:25 +0000
+++ src/compiler/api/compilercb.cpp	2012-01-18 11:01:27 +0000
@@ -27,6 +27,7 @@
 #include "system/properties.h"
 
 #include "zorbaserialization/serialization_engine.h"
+#include "functions/udf.h"
 
 
 namespace zorba 
@@ -120,6 +121,7 @@
   theTimeout(timeout),
   theTempIndexCounter(0)
 {
+  theLocalUdfs = new rclist<user_function*>;
 }
 
 
@@ -143,6 +145,7 @@
   theTempIndexCounter(0),
   theConfig(cb.theConfig)
 {
+  theLocalUdfs = new rclist<user_function*>;
 }
 
 
@@ -169,6 +172,20 @@
 {
 }
 
+//compile all the user_functions so the expr tree is stable at serialize
+void CompilerCB::prepare_for_serialize()
+{
+  rclist<user_function*>::iterator udf_it;
+  for(udf_it=theLocalUdfs->begin(); udf_it != theLocalUdfs->end(); udf_it++)
+  {
+    (*udf_it)->prepare_for_serialize(this);
+  }
+}
+
+rchandle<rclist<user_function*> >  CompilerCB::get_local_udfs()
+{
+  return theLocalUdfs;
+}
 
 /*******************************************************************************
 

=== modified file 'src/compiler/api/compilercb.h'
--- src/compiler/api/compilercb.h	2012-01-11 17:30:25 +0000
+++ src/compiler/api/compilercb.h	2012-01-18 11:01:27 +0000
@@ -29,7 +29,7 @@
 // without having the definition of static_context availble.
 # include "context/static_context.h"
 #endif
-
+#include "zorbatypes/rclist.h"
 #include "zorbaserialization/class_serializer.h"
 
 namespace zorba {
@@ -39,7 +39,6 @@
 #endif
 class static_context;
 
-
 /*******************************************************************************
   There is one CompilerCB per query plus one CompilerCB per invocation of an
   eval or xqdoc expression that appears in the query. The query-level ccb is
@@ -167,10 +166,14 @@
 
   config                    theConfig;
 
+  rchandle<rclist<user_function*> >    theLocalUdfs;//for plan serializer
+
 public:
   SERIALIZABLE_CLASS(CompilerCB);
   CompilerCB(::zorba::serialization::Archiver& ar);
   void serialize(::zorba::serialization::Archiver& ar);
+  void prepare_for_serialize();
+  rchandle<rclist<user_function*> >  get_local_udfs();
 
 public:
   CompilerCB(XQueryDiagnostics*, long timeout = -1);

=== modified file 'src/compiler/expression/var_expr.cpp'
--- src/compiler/expression/var_expr.cpp	2012-01-11 17:30:25 +0000
+++ src/compiler/expression/var_expr.cpp	2012-01-18 11:01:27 +0000
@@ -134,6 +134,7 @@
   ar & theCopyClause;
   ar & theParamPos;
   ar & theUDF;
+  ar & theSetExprs;
   ar & theIsPrivate;
   ar & theIsExternal;
   ar & theIsMutable;

=== modified file 'src/compiler/translator/translator.cpp'
--- src/compiler/translator/translator.cpp	2012-01-11 17:30:25 +0000
+++ src/compiler/translator/translator.cpp	2012-01-18 11:01:27 +0000
@@ -3331,7 +3331,7 @@
     }
     else // Process UDF (non-external) function declaration
     {
-      f = new user_function(loc, sig, NULL, scriptKind); // no body for now
+      f = new user_function(loc, sig, NULL, scriptKind, theCCB); // no body for now
     }
 
     f->setAnnotations(theAnnotations);
@@ -10424,7 +10424,8 @@
     user_function* udf = new user_function(loc,
                                            fn->getSignature(),
                                            NULL, // no body for now
-                                           fn->getScriptingKind());
+                                           fn->getScriptingKind(),
+                                           theCCB);
 
     std::vector<expr_t> foArgs(arity);
     std::vector<var_expr_t> udfArgs(arity);
@@ -10632,7 +10633,8 @@
   user_function_t udf(new user_function(loc,
                                         signature(0, paramTypes, returnType),
                                         body.getp(),
-                                        body->get_scripting_detail()));
+                                        body->get_scripting_detail(),
+                                        theCCB));
   udf->setArgVars(argVars);
   udf->setOptimized(true);
 

=== modified file 'src/context/static_context.cpp'
--- src/context/static_context.cpp	2012-01-11 17:30:25 +0000
+++ src/context/static_context.cpp	2012-01-18 11:01:27 +0000
@@ -22,6 +22,7 @@
 #include <zorba/external_module.h>
 #include <zorba/serialization_callback.h>
 
+#include "functions/udf.h"
 #include "zorbaserialization/serialization_engine.h"
 
 #include "zorbamisc/ns_consts.h"

=== modified file 'src/context/static_context.h'
--- src/context/static_context.h	2012-01-11 17:30:25 +0000
+++ src/context/static_context.h	2012-01-18 11:01:27 +0000
@@ -599,6 +599,8 @@
 
   void serialize(serialization::Archiver& ar);
 
+  void prepare_for_serialize(CompilerCB *compiler_cb);
+
 public:
   static_context(::zorba::serialization::Archiver& ar);
 

=== modified file 'src/functions/udf.cpp'
--- src/functions/udf.cpp	2012-01-14 21:58:23 +0000
+++ src/functions/udf.cpp	2012-01-18 11:01:27 +0000
@@ -50,7 +50,8 @@
     const QueryLoc& loc,
     const signature& sig,
     expr_t expr_body,
-    short scriptingKind)
+    short scriptingKind,
+    CompilerCB  *compilerCB)
   :
   function(sig, FunctionConsts::FN_UNKNOWN),
   theLoc(loc),
@@ -68,6 +69,8 @@
   resetFlag(FunctionConsts::isBuiltin);
   setDeterministic(true);
   setPrivate(false);
+  theLocalUdfs = compilerCB->get_local_udfs();
+  theLocalUdfs->push_back(this);
 }
 
 
@@ -88,8 +91,22 @@
 ********************************************************************************/
 user_function::~user_function()
 {
-}
-
+  if(theLocalUdfs != NULL)
+    theLocalUdfs->remove(this);
+}
+
+
+void user_function::prepare_for_serialize(CompilerCB *compilerCB)
+{
+  uint32_t planStateSize;
+  getPlan(compilerCB, planStateSize);
+  std::vector<user_function*>::iterator udf_it;
+  for(udf_it=theMutuallyRecursiveUDFs.begin(); udf_it!=theMutuallyRecursiveUDFs.end();udf_it++)
+  {
+    if((*udf_it)->thePlan == NULL)
+      (*udf_it)->prepare_for_serialize(compilerCB);
+  }
+}
 
 /*******************************************************************************
 
@@ -124,8 +141,10 @@
         getPlan(ar.compiler_cb);
       }
 #else
-      uint32_t planStateSize;
-      getPlan(ar.compiler_cb, planStateSize);
+      //uint32_t planStateSize;
+      //getPlan(ar.compiler_cb, planStateSize);
+      assert(thePlan != NULL);
+      ZORBA_ASSERT(thePlan != NULL);
 #endif
     }
     catch(...)

=== modified file 'src/functions/udf.h'
--- src/functions/udf.h	2012-01-14 21:58:23 +0000
+++ src/functions/udf.h	2012-01-18 11:01:27 +0000
@@ -20,6 +20,7 @@
 #include "functions/function.h"
 
 #include "compiler/expression/expr_base.h"
+#include "zorbatypes/rclist.h"
 
 
 namespace zorba 
@@ -131,17 +132,21 @@
   bool                        theCacheResults;
   bool                        theCacheComputed;
 
+  rchandle<rclist<user_function*> >    theLocalUdfs;//for plan serializer
+
 public:
   SERIALIZABLE_CLASS(user_function)
   user_function(::zorba::serialization::Archiver& ar);
   void serialize(::zorba::serialization::Archiver& ar);
+  void prepare_for_serialize(CompilerCB *compilerCB);
 
 public:
   user_function(
       const QueryLoc& loc,
       const signature& sig,
       expr_t expr_body,
-      short kind);
+      short kind,
+      CompilerCB  *compilerCB);
 
   virtual ~user_function();
 

=== modified file 'src/zorbaserialization/archiver.cpp'
--- src/zorbaserialization/archiver.cpp	2011-07-13 01:56:45 +0000
+++ src/zorbaserialization/archiver.cpp	2012-01-18 11:01:27 +0000
@@ -27,6 +27,7 @@
 #include "class_serializer.h"
 #include "mem_archiver.h"
 
+
 namespace zorba
 {
 
@@ -112,7 +113,7 @@
     current_level(0),
     internal_archive(internal_archive),
     theUserCallback(0),
-    compiler_cb(0),
+    //compiler_cb(0),
     dont_allow_delay_for_plan_sctx(false)
 {
 
@@ -195,6 +196,7 @@
     ref_field = check_nonclass_pointer(type, orig_ptr);
   if(ref_field)
   {
+    assert((field_treat != ARCHIVE_FIELD_NORMAL) || (ref_field->field_treat != ARCHIVE_FIELD_NORMAL));
     if(get_is_temp_field_one_level() && (field_treat == ARCHIVE_FIELD_IS_PTR) && (allow_delay2 == ALLOW_DELAY))
       allow_delay2 = DONT_ALLOW_DELAY;
     if(field_treat == ARCHIVE_FIELD_NORMAL)
@@ -309,6 +311,7 @@
   }
   if(ref_field)
   {
+    assert((field_treat != ARCHIVE_FIELD_NORMAL) || (ref_field->field_treat != ARCHIVE_FIELD_NORMAL));
     if(get_is_temp_field_one_level() && (field_treat == ARCHIVE_FIELD_IS_PTR) && (allow_delay2 == ALLOW_DELAY))
       allow_delay2 = DONT_ALLOW_DELAY;
     if(field_treat == ARCHIVE_FIELD_NORMAL)

=== modified file 'src/zorbaserialization/archiver.h'
--- src/zorbaserialization/archiver.h	2011-10-03 09:18:49 +0000
+++ src/zorbaserialization/archiver.h	2012-01-18 11:01:27 +0000
@@ -202,7 +202,7 @@
   SerializationCallback*      theUserCallback;
 
 public:
-  CompilerCB  *compiler_cb;///to workaround user defined function compile-at-runtime
+  //CompilerCB  *compiler_cb;///to workaround user defined function compile-at-runtime
   bool        dont_allow_delay_for_plan_sctx;
 public:
   Archiver(bool is_serializing_out, bool internal_archive=false);

=== modified file 'src/zorbaserialization/zorba_class_serializer.h'
--- src/zorbaserialization/zorba_class_serializer.h	2011-07-07 12:05:43 +0000
+++ src/zorbaserialization/zorba_class_serializer.h	2012-01-18 11:01:27 +0000
@@ -28,6 +28,7 @@
 #include "zorbaserialization/archiver.h"
 
 #include "store/api/shared_types.h"
+#include "diagnostics/assert.h"
 
 #include <stdio.h>
 #include <map>
@@ -224,7 +225,8 @@
     bool is_ref;
     ENUM_ALLOW_DELAY allow_delay = ar.get_allow_delay();
     is_ref = ar.add_compound_field("rchandle<T>", 0, !FIELD_IS_CLASS, "", &obj, ARCHIVE_FIELD_NORMAL);
-    if(!is_ref)
+    assert(!is_ref);
+    ZORBA_ASSERT(!is_ref);
     {
       T *p = obj.getp();
       if(allow_delay != ALLOW_DELAY)
@@ -242,10 +244,6 @@
         ar.set_is_temp_field_one_level(false);
       ar.add_end_compound_field();
     }
-    else
-    {
-      assert(false);
-    }
   }
   else
   {

=== modified file 'src/zorbatypes/rchandle.h'
--- src/zorbatypes/rchandle.h	2011-07-12 20:15:01 +0000
+++ src/zorbatypes/rchandle.h	2012-01-18 11:01:27 +0000
@@ -92,9 +92,9 @@
 
   long getRefCount() const { return theRefCount; }
 
-  long* getSharedRefCounter() const { ZORBA_FATAL(0, ""); return NULL; } 
+  //long* getSharedRefCounter() const { ZORBA_FATAL(0, ""); return NULL; } 
  
-  SYNC_CODE(RCLock* getRCLock() const { ZORBA_FATAL(0, ""); return NULL; });
+  //SYNC_CODE(RCLock* getRCLock() const { ZORBA_FATAL(0, ""); return NULL; });
 
   void addReference(long* sharedCounter SYNC_PARAM2(RCLock* lock)) const;
 

=== added file 'src/zorbatypes/rclist.h'
--- src/zorbatypes/rclist.h	1970-01-01 00:00:00 +0000
+++ src/zorbatypes/rclist.h	2012-01-18 11:01:27 +0000
@@ -0,0 +1,37 @@
+/*
+ * Copyright 2006-2008 The FLWOR Foundation.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include <list>
+#include "common/common.h"
+#include "zorbatypes/rchandle.h"
+#pragma once
+#ifndef ZORBA_REF_COUNTED_STD_LIST
+#define ZORBA_REF_COUNTED_STD_LIST
+
+namespace zorba
+{
+
+template <typename T>
+class rclist : public RCObject, public std::list<T>
+{
+  SYNC_CODE(mutable RCLock  lock;)
+public:
+  long* getSharedRefCounter() const { return NULL; }
+  SYNC_CODE(RCLock* getRCLock() const { return &lock; });
+};
+
+}//end namespace zorba
+
+#endif