Changeset: 4621230459b4 for MonetDB
URL: https://dev.monetdb.org/hg/MonetDB/rev/4621230459b4
Modified Files:
gdk/gdk_bat.c
gdk/gdk_batop.c
gdk/gdk_bbp.c
gdk/gdk_utils.c
sql/backends/monet5/sql_statistics.c
sql/storage/bat/bat_storage.c
Branch: default
Log Message:
Remove some data races.
Don't use BBPlock during exit: we're the only thread at this time
anyway;
Use the heap lock instead of index lock in BATordered(_rev);
Don't set bat to read-only on every access in cs_bind_bat, but just make
sure it's already read-only before we get there.
diffs (240 lines):
diff --git a/gdk/gdk_bat.c b/gdk/gdk_bat.c
--- a/gdk/gdk_bat.c
+++ b/gdk/gdk_bat.c
@@ -1876,9 +1876,10 @@ BATsetcount(BAT *b, BUN cnt)
MT_lock_set(&b->theaplock);
b->batCount = cnt;
b->batDirtydesc = true;
- b->theap->dirty |= b->ttype != TYPE_void && b->theap->parentid ==
b->batCacheid && cnt > 0;
- if (b->theap->parentid == b->batCacheid)
+ if (b->theap->parentid == b->batCacheid) {
+ b->theap->dirty |= b->ttype != TYPE_void && cnt > 0;
b->theap->free = tailsize(b, cnt);
+ }
if (b->ttype == TYPE_void)
b->batCapacity = cnt;
if (cnt <= 1) {
diff --git a/gdk/gdk_batop.c b/gdk/gdk_batop.c
--- a/gdk/gdk_batop.c
+++ b/gdk/gdk_batop.c
@@ -1863,12 +1863,15 @@ BATordered(BAT *b)
if (b->tnosorted > 0 || !ATOMlinear(b->ttype))
return false;
- /* In order that multiple threads don't scan the same BAT at the
- * same time (happens a lot with mitosis/mergetable), we use a
- * lock. We reuse the batIdxLock lock for this, not because this
- * scanning interferes with heap reference counting, but because
- * it's there, and not so likely to be used at the same time. */
- MT_lock_set(&b->batIdxLock);
+ /* There are a few reasons why we need a lock here. It may be
+ * that multiple threads call this functions at the same time
+ * (happens a lot with mitosis/mergetable), but we only need to
+ * scan the bat in one thread: the others can reap the rewards
+ * when that one thread is done. Also, we need the heap to
+ * remain accessible (could have used bat_iterator for that),
+ * and, and this is the killer argument, we may need to make
+ * changes to the bat descriptor. */
+ MT_lock_set(&b->theaplock);
BATiter bi = bat_iterator_nolock(b);
if (!b->tsorted && b->tnosorted == 0) {
b->batDirtydesc = true;
@@ -1969,7 +1972,7 @@ BATordered(BAT *b)
}
}
doreturn:
- MT_lock_unset(&b->batIdxLock);
+ MT_lock_unset(&b->theaplock);
return b->tsorted;
}
@@ -2015,7 +2018,7 @@ BATordered_rev(BAT *b)
return is_oid_nil(b->tseqbase);
if (BATtdense(b) || b->tnorevsorted > 0)
return false;
- MT_lock_set(&b->batIdxLock);
+ MT_lock_set(&b->theaplock);
BATiter bi = bat_iterator_nolock(b);
if (!b->trevsorted && b->tnorevsorted == 0) {
b->batDirtydesc = true;
@@ -2059,7 +2062,7 @@ BATordered_rev(BAT *b)
TRC_DEBUG(ALGO, "Fixed revsorted for " ALGOBATFMT " (" LLFMT "
usec)\n", ALGOBATPAR(b), GDKusec() - t0);
}
doreturn:
- MT_lock_unset(&b->batIdxLock);
+ MT_lock_unset(&b->theaplock);
return b->trevsorted;
}
diff --git a/gdk/gdk_bbp.c b/gdk/gdk_bbp.c
--- a/gdk/gdk_bbp.c
+++ b/gdk/gdk_bbp.c
@@ -1753,7 +1753,7 @@ BBPexit(void)
bat i;
bool skipped;
- BBPlock(); /* stop all threads ever touching more descriptors */
+ //BBPlock(); /* stop all threads ever touching more descriptors */
/* free all memory (just for leak-checking in Purify) */
do {
@@ -2597,8 +2597,12 @@ bbpclear(bat i, bool lock)
MT_lock_set(&GDKcacheLock);
BBP_status_set(i, BBPUNLOADING);
+ if (lock)
+ MT_lock_set(&GDKswapLock(i));
BBP_refs(i) = 0;
BBP_lrefs(i) = 0;
+ if (lock)
+ MT_lock_unset(&GDKswapLock(i));
if (!BBPtmpcheck(BBP_logical(i))) {
MT_lock_set(&BBPnameLock);
BBP_delete(i);
@@ -2977,6 +2981,8 @@ decref(bat i, bool logical, bool release
/* only consider unloading if refs is 0; if, in addition, lrefs
* is 0, we can definitely unload, else only if some more
* conditions are met */
+ if (b)
+ MT_lock_set(&b->theaplock);
if (BBP_refs(i) == 0 &&
(BBP_lrefs(i) == 0 ||
(b != NULL && b->theap != NULL
@@ -2999,6 +3005,8 @@ decref(bat i, bool logical, bool release
swap = true;
} /* else: bat cannot be swapped out */
lrefs = BBP_lrefs(i);
+ if (b)
+ MT_lock_unset(&b->theaplock);
/* unlock before re-locking in unload; as saving a dirty
* persistent bat may take a long time */
diff --git a/gdk/gdk_utils.c b/gdk/gdk_utils.c
--- a/gdk/gdk_utils.c
+++ b/gdk/gdk_utils.c
@@ -950,7 +950,7 @@ GDKinit(opt *set, int setlen, bool embed
}
} else {
/* BBP was locked by BBPexit() */
- BBPunlock();
+ //BBPunlock();
}
GDKtracer_init(dbpath, dbtrace);
errno = 0;
diff --git a/sql/backends/monet5/sql_statistics.c
b/sql/backends/monet5/sql_statistics.c
--- a/sql/backends/monet5/sql_statistics.c
+++ b/sql/backends/monet5/sql_statistics.c
@@ -132,11 +132,8 @@ sql_analyze(Client cntxt, MalBlkPtr mb,
}
/* Collect new sorted and revsorted
properties */
- /* At the moment the heap must be
locked around the BATordered calls to prevent a race condition on it */
- MT_lock_set(&b->theaplock);
(void) BATordered(b);
(void) BATordered_rev(b);
- MT_lock_unset(&b->theaplock);
/* Check for nils existence */
(void) BATcount_no_nil(b, NULL);
diff --git a/sql/storage/bat/bat_storage.c b/sql/storage/bat/bat_storage.c
--- a/sql/storage/bat/bat_storage.c
+++ b/sql/storage/bat/bat_storage.c
@@ -480,6 +480,7 @@ temp_dup_cs(column_storage *cs, ulng tid
BAT *b = bat_new(type, 1024, TRANSIENT);
if (!b)
return LOG_ERR;
+ bat_set_access(b, BAT_READ);
cs->bid = temp_create(b);
bat_destroy(b);
cs->uibid = e_bat(TYPE_oid);
@@ -988,7 +989,7 @@ cs_bind_bat( column_storage *cs, int acc
b = temp_descriptor(cs->bid);
if (b == NULL)
return NULL;
- bat_set_access(b, BAT_READ);
+ assert(b->batRestricted == BAT_READ);
/* return slice */
BAT *s = BATslice(b, 0, cnt);
bat_destroy(b);
@@ -1176,6 +1177,7 @@ dict_append_bat(sql_trans *tr, sql_delta
}
if (cs->bid && !new)
temp_destroy(cs->bid);
+ bat_set_access(n, BAT_READ);
cs->bid = temp_create(n);
bat_destroy(n);
if (cs->ebid && !new)
@@ -1217,6 +1219,7 @@ dict_append_bat(sql_trans *tr, sql_delta
}
if (cs->bid && !new)
temp_destroy(cs->bid);
+ bat_set_access(n, BAT_READ);
cs->bid = temp_create(n);
bat_destroy(n);
cs->cleared = true;
@@ -1268,6 +1271,7 @@ for_append_bat(column_storage *cs, BAT *
return NULL;
if (cs->bid)
temp_destroy(cs->bid);
+ bat_set_access(n, BAT_READ);
cs->bid = temp_create(n);
cs->ucnt = 0;
if (cs->uibid)
@@ -1702,6 +1706,7 @@ dict_append_val(sql_trans *tr, sql_delta
}
if (cs->bid && !new)
temp_destroy(cs->bid);
+ bat_set_access(n, BAT_READ);
cs->bid = temp_create(n);
bat_destroy(n);
if (cs->ebid && !new)
@@ -1738,6 +1743,7 @@ dict_append_val(sql_trans *tr, sql_delta
}
if (cs->bid)
temp_destroy(cs->bid);
+ bat_set_access(n, BAT_READ);
cs->bid = temp_create(n);
bat_destroy(n);
cs->cleared = true;
@@ -1773,6 +1779,7 @@ for_append_val(column_storage *cs, void
/* TODO decompress updates if any */
if (cs->bid)
temp_destroy(cs->bid);
+ bat_set_access(n, BAT_READ);
cs->bid = temp_create(n);
cs->st = ST_DEFAULT;
/* at append_col the column's storage type is cleared */
@@ -1881,7 +1888,7 @@ dup_cs(sql_trans *tr, column_storage *oc
cs->ucnt = ocs->ucnt;
if (temp) {
- cs->bid = temp_copy(cs->bid, true, true);
+ cs->bid = temp_copy(cs->bid, true, false);
if (cs->bid == BID_NIL)
return LOG_ERR;
} else {
@@ -3526,7 +3533,7 @@ clear_cs(sql_trans *tr, column_storage *
cs->bid = temp_create(n); /* create empty copy
*/
bat_destroy(n);
} else {
- bat nbid = temp_copy(cs->bid, true, temp); /*
create empty copy */
+ bat nbid = temp_copy(cs->bid, true, false); /*
create empty copy */
if (nbid == BID_NIL)
return BUN_NONE;
@@ -4774,6 +4781,7 @@ swap_bats(sql_trans *tr, sql_column *col
temp_destroy(d->cs.uibid);
if (d->cs.uvbid)
temp_destroy(d->cs.uvbid);
+ bat_set_access(bn, BAT_READ);
d->cs.bid = temp_create(bn);
d->cs.uibid = 0;
d->cs.uvbid = 0;
@@ -4804,6 +4812,7 @@ col_compress(sql_trans *tr, sql_column *
d->cs.cleared = true;
if (d->cs.bid)
temp_destroy(d->cs.bid);
+ bat_set_access(o, BAT_READ);
d->cs.bid = temp_create(o);
if (u) {
if (d->cs.ebid)
_______________________________________________
checkin-list mailing list -- [email protected]
To unsubscribe send an email to [email protected]
