Changeset: e2fc8bd027c1 for MonetDB
URL: https://dev.monetdb.org/hg/MonetDB/rev/e2fc8bd027c1
Modified Files:
gdk/gdk.h
gdk/gdk_bat.c
gdk/gdk_batop.c
gdk/gdk_bbp.c
gdk/gdk_group.c
gdk/gdk_hash.c
gdk/gdk_select.c
monetdb5/mal/mal_profiler.c
Branch: default
Log Message:
Merge with Jul2021 branch.
diffs (truncated from 1105 to 300 lines):
diff --git a/gdk/gdk.h b/gdk/gdk.h
--- a/gdk/gdk.h
+++ b/gdk/gdk.h
@@ -871,7 +871,40 @@ gdk_export size_t HEAPmemsize(Heap *h);
gdk_export void HEAPdecref(Heap *h, bool remove);
gdk_export void HEAPincref(Heap *h);
-/* BAT iterator, also protects use of BAT heaps with reference counts */
+/* BAT iterator, also protects use of BAT heaps with reference counts.
+ *
+ * A BAT iterator has to be used with caution, but it does have to be
+ * used in many place.
+ *
+ * An iterator is initialized by assigning it the result of a call to
+ * either bat_iterator or bat_iterator_nolock. The former must be
+ * accompanied by a call to bat_iterator_end to release resources.
+ *
+ * bat_iterator should be used for BATs that could possibly be modified
+ * in another thread while we're reading the contents of the BAT.
+ * Alternatively, but only for very quick access, the theaplock can be
+ * taken, the data read, and the lock released. For longer duration
+ * accesses, it is better to use the iterator, even without the BUNt*
+ * macros, since the theaplock is only held very briefly.
+ *
+ * If BATs are to be modified, higher level code must assure that no
+ * other thread is going to modify the same BAT at the same time. A
+ * to-be-modified BAT should not use bat_iterator. It can use
+ * bat_iterator_nolock, but be aware that this creates a copy of the
+ * heap pointer(s) (i.e. theap and tvheap) and if the heaps get
+ * extended, the pointers in the BAT structure may be modified, but that
+ * does not modify the pointers in the iterator. This means that after
+ * operations that may grow a heap, the iterator should be
+ * reinitialized.
+ *
+ * The BAT iterator provides a number of fields that can (and often
+ * should) be used to access information about the BAT. For string
+ * BATs, if a parallel threads adds values, the offset heap (theap) may
+ * get replaced by a one that is wider. This involves changing the
+ * twidth and tshift values in the BAT structure. These changed values
+ * should not be used to access the data in the iterator. Instead, use
+ * the width and shift values in the iterator itself.
+ */
typedef struct BATiter {
BAT *b;
Heap *h;
diff --git a/gdk/gdk_bat.c b/gdk/gdk_bat.c
--- a/gdk/gdk_bat.c
+++ b/gdk/gdk_bat.c
@@ -613,14 +613,18 @@ BATclear(BAT *b, bool force)
if (b->tvheap && b->tvheap->free > 0) {
Heap *th = GDKmalloc(sizeof(Heap));
- if (th == NULL)
+ if (th == NULL) {
+ MT_lock_unset(&b->theaplock);
return GDK_FAIL;
+ }
*th = (Heap) {
.farmid = b->tvheap->farmid,
};
strcpy_len(th->filename, b->tvheap->filename,
sizeof(th->filename));
- if (ATOMheap(b->ttype, th, 0) != GDK_SUCCEED)
+ if (ATOMheap(b->ttype, th, 0) != GDK_SUCCEED) {
+ MT_lock_unset(&b->theaplock);
return GDK_FAIL;
+ }
ATOMIC_INIT(&th->refs, 1);
th->parentid = b->tvheap->parentid;
th->dirty = true;
@@ -1447,9 +1451,9 @@ BUNinplacemulti(BAT *b, const oid *posit
MT_rwlock_wrunlock(&b->thashlock);
return GDK_FAIL;
}
- /* reinitialize iterator after heap upgrade */
- bi = bat_iterator_nolock(b);
}
+ /* reinitialize iterator after possible heap upgrade */
+ bi = bat_iterator_nolock(b);
_ptr = BUNtloc(bi, p);
switch (b->twidth) {
default: /* only three or four cases possible */
diff --git a/gdk/gdk_batop.c b/gdk/gdk_batop.c
--- a/gdk/gdk_batop.c
+++ b/gdk/gdk_batop.c
@@ -1298,8 +1298,11 @@ BATappend_or_update(BAT *b, BAT *p, BAT
bat_iterator_end(&ni);
return GDK_FAIL;
}
- bi = bat_iterator_nolock(b);
}
+ /* in case ATOMreplaceVAR and/or
+ * GDKupgradevarheap replaces a heap, we need to
+ * reinitialize the iterator */
+ bi = bat_iterator_nolock(b);
switch (b->twidth) {
case 1:
((uint8_t *) b->theap->base)[updid] = (uint8_t)
(d - GDK_VAROFFSET);
@@ -3036,7 +3039,7 @@ BATcount_no_nil(BAT *b, BAT *s)
break;
case TYPE_str:
base = bi.vh->base;
- switch (b->twidth) {
+ switch (bi.width) {
case 1:
for (i = 0; i < n; i++)
cnt += base[(var_t) ((const unsigned char *)
p)[canditer_next(&ci) - hseq] + GDK_VAROFFSET] != '\200';
diff --git a/gdk/gdk_bbp.c b/gdk/gdk_bbp.c
--- a/gdk/gdk_bbp.c
+++ b/gdk/gdk_bbp.c
@@ -1764,7 +1764,7 @@ BBPdir_step(bat bid, BUN size, int n, ch
GDKerror("Writing BBP.dir file failed.\n");
goto bailout;
}
- if (fgets(buf, bufsize, *obbpfp) == NULL) {
+ if (fgets(buf, (int) bufsize, *obbpfp) == NULL) {
if (ferror(*obbpfp)) {
GDKerror("error reading backup BBP.dir.");
goto bailout;
@@ -1803,7 +1803,7 @@ BBPdir_last(int n, char *buf, size_t buf
goto bailout;
}
while (obbpf) {
- if (fgets(buf, bufsize, obbpf) == NULL) {
+ if (fgets(buf, (int) bufsize, obbpf) == NULL) {
if (ferror(obbpf)) {
GDKerror("error reading backup BBP.dir.");
goto bailout;
@@ -3519,13 +3519,11 @@ BBPsync(int cnt, bat *restrict subcommit
if (d)
MT_lock_set(&d->theaplock);
-// else
-// MT_lock_set(&GDKswapLock(i));
if (BBP_status(i) & BBPPERSISTENT) {
BAT *b = dirty_bat(&i, subcommit != NULL);
if (i <= 0) {
-// MT_lock_unset(&GDKswapLock(subcommit ?
subcommit[idx] : idx));
- MT_lock_unset(&BBP_desc(i)->theaplock);
+ if (d)
+ MT_lock_unset(&d->theaplock);
break;
}
if (b)
@@ -3536,8 +3534,6 @@ BBPsync(int cnt, bat *restrict subcommit
}
if (d)
MT_lock_unset(&d->theaplock);
-// else
-// MT_lock_unset(&GDKswapLock(i));
if (n == -2)
break;
/* we once again have a saved heap */
diff --git a/gdk/gdk_group.c b/gdk/gdk_group.c
--- a/gdk/gdk_group.c
+++ b/gdk/gdk_group.c
@@ -796,14 +796,17 @@ BATgroup_internal(BAT **groups, BAT **ex
if (gn == NULL)
goto error;
ngrps = (oid *) Tloc(gn, 0);
+ maxgrps = BUN_NONE;
MT_rwlock_rdlock(&b->thashlock);
if (b->thash && b->thash != (Hash *) 1)
maxgrps = b->thash->nunique;
- else if ((prop = BATgetprop_nolock(b, GDK_NUNIQUE)) != NULL)
- maxgrps = prop->val.oval;
- else
- maxgrps = cnt / 10;
MT_rwlock_rdunlock(&b->thashlock);
+ if (maxgrps == BUN_NONE) {
+ if ((prop = BATgetprop(b, GDK_NUNIQUE)) != NULL)
+ maxgrps = prop->val.oval;
+ else
+ maxgrps = cnt / 10;
+ }
if (!is_oid_nil(maxgrp) && maxgrps < maxgrp)
maxgrps += maxgrp;
if (e && maxgrps < BATcount(e))
@@ -812,18 +815,20 @@ BATgroup_internal(BAT **groups, BAT **ex
maxgrps += BATcount(h);
if (maxgrps < GROUPBATINCR)
maxgrps = GROUPBATINCR;
- if (b->twidth <= 2)
- maxgrps = (BUN) 1 << (8 * b->twidth);
+ bi = bat_iterator(b);
+
+ if (bi.width <= 2)
+ maxgrps = (BUN) 1 << (8 * bi.width);
if (extents) {
en = COLnew(0, TYPE_oid, maxgrps, TRANSIENT);
if (en == NULL)
- goto error;
+ goto error1;
exts = (oid *) Tloc(en, 0);
}
if (histo) {
hn = COLnew(0, TYPE_lng, maxgrps, TRANSIENT);
if (hn == NULL)
- goto error;
+ goto error1;
cnts = (lng *) Tloc(hn, 0);
}
ngrp = 0;
@@ -837,8 +842,8 @@ BATgroup_internal(BAT **groups, BAT **ex
/* for strings we can use the offset instead of the actual
* string values if we know that the strings in the string
* heap are unique */
- if (t == TYPE_str && GDK_ELIMDOUBLES(b->tvheap)) {
- switch (b->twidth) {
+ if (t == TYPE_str && GDK_ELIMDOUBLES(bi.vh)) {
+ switch (bi.width) {
case 1:
t = TYPE_bte;
break;
@@ -858,8 +863,6 @@ BATgroup_internal(BAT **groups, BAT **ex
}
}
- bi = bat_iterator(b);
-
if (subsorted ||
((BATordered(b) || BATordered_rev(b)) &&
(g == NULL || BATordered(g) || BATordered_rev(g)))) {
@@ -925,7 +928,7 @@ BATgroup_internal(BAT **groups, BAT **ex
/* array to maintain last time we saw each old group */
pgrp = GDKmalloc(sizeof(BUN) * j);
if (pgrp == NULL)
- goto error;
+ goto error1;
/* initialize to impossible position */
memset(pgrp, ~0, sizeof(BUN) * j);
@@ -972,7 +975,7 @@ BATgroup_internal(BAT **groups, BAT **ex
algomsg = "byte-sized groups -- ";
if (bgrps == NULL)
- goto error;
+ goto error1;
memset(bgrps, 0xFF, 256);
if (histo)
memset(cnts, 0, maxgrps * sizeof(lng));
@@ -1007,7 +1010,7 @@ BATgroup_internal(BAT **groups, BAT **ex
algomsg = "short-sized groups -- ";
if (sgrps == NULL)
- goto error;
+ goto error1;
memset(sgrps, 0xFF, 65536 * sizeof(short));
if (histo)
memset(cnts, 0, maxgrps * sizeof(lng));
@@ -1055,10 +1058,12 @@ BATgroup_internal(BAT **groups, BAT **ex
* calculate the bounds [lo, lo+BATcount(b))
* in the parent that b uses */
BAT *b2 = BBPdescriptor(parent);
+ MT_rwlock_rdunlock(&b->thashlock);
lo = b->tbaseoff - b2->tbaseoff;
b = b2;
bat_iterator_end(&bi);
bi = bat_iterator(b);
+ MT_rwlock_rdlock(&b->thashlock);
algomsg = "existing parent hash -- ";
phash = true;
}
@@ -1121,7 +1126,7 @@ BATgroup_internal(BAT **groups, BAT **ex
* BATassertProps for similar code; we also exploit if
* g is clustered */
algomsg = "new partial hash -- ";
- nme = GDKinmemory(b->theap->farmid) ? ":memory:" :
BBP_physical(b->batCacheid);
+ nme = GDKinmemory(bi.h->farmid) ? ":memory:" :
BBP_physical(b->batCacheid);
if (grps && !gc) {
/* we manipulate the hash value after having
* calculated it, and when doing that, we
@@ -1159,7 +1164,7 @@ BATgroup_internal(BAT **groups, BAT **ex
GDKfree(hs);
hs = NULL;
GDKerror("cannot allocate hash table\n");
- goto error;
+ goto error1;
}
if (snprintf(hs->heaplink.filename,
sizeof(hs->heaplink.filename), "%s.thshgrpl%x", nme, (unsigned) THRgettid()) >=
(int) sizeof(hs->heaplink.filename) ||
snprintf(hs->heapbckt.filename,
sizeof(hs->heapbckt.filename), "%s.thshgrpb%x", nme, (unsigned) THRgettid()) >=
(int) sizeof(hs->heapbckt.filename) ||
@@ -1167,7 +1172,7 @@ BATgroup_internal(BAT **groups, BAT **ex
GDKfree(hs);
hs = NULL;
GDKerror("cannot allocate hash table\n");
- goto error;
+ goto error1;
}
gn->tsorted = true; /* be optimistic */
@@ -1305,8 +1310,9 @@ BATgroup_internal(BAT **groups, BAT **ex
ALGOOPTBATPAR(gn), ALGOOPTBATPAR(en),
ALGOOPTBATPAR(hn), algomsg, GDKusec() - t0);
return GDK_SUCCEED;
+ error1:
+ bat_iterator_end(&bi);
error:
- bat_iterator_end(&bi);
if (hs != NULL && hs != b->thash) {
HEAPfree(&hs->heaplink, true);
HEAPfree(&hs->heapbckt, true);
_______________________________________________
checkin-list mailing list
[email protected]
https://www.monetdb.org/mailman/listinfo/checkin-list
